Feat: add risingwave engine adapter support for sqlmesh. #3436
Conversation
| COMMENT_CREATION_VIEW = CommentCreationView.COMMENT_COMMAND_ONLY | ||
| SUPPORTS_MATERIALIZED_VIEWS = True | ||
|
|
||
| SCHEMA_DIFFER = SchemaDiffer( |
There was a problem hiding this comment.
This appears to be copy+pasted directly from the Postgres adapter. Rather than duplicating it, can you extend it instead?
| """Begin a new session.""" | ||
| self._set_flush() | ||
|
|
||
| def _fetch_native_df( |
There was a problem hiding this comment.
This also looks like it was copy+pasted directly from the Postgres adapter...
There was a problem hiding this comment.
Yes, because risingwave is postgres-compatible, which means postgres engine adapter works for risingwave with few changes. Do you have an idea on how to support it without manually copying?
There was a problem hiding this comment.
So instead of copy+pasting the PostgresEngineAdapter class definition:
class RisingwaveEngineAdapter(
BasePostgresEngineAdapter,
PandasNativeFetchDFSupportMixin,
GetCurrentCatalogFromFunctionMixin,
):
Is there a reason you cant extend it instead?
class RisingwaveEngineAdapter(PostgresEngineAdapter):
| @@ -89,6 +89,14 @@ gateways: | |||
| cluster: cluster1 | |||
| state_connection: | |||
| type: duckdb | |||
| inttest_risingwave: | |||
There was a problem hiding this comment.
For the integration tests to run on CircleCI without the port 5432 failed: server closed the connection unexpectedly error, you'll also need to add a branch for risingwave to .circleci/wait-for-db.sh to block until the DB server is available
There was a problem hiding this comment.
Hi Erin, thanks for your help, I have updated and now there are some issues with the integration tests and I am trying to fix them.
| @@ -1672,6 +1672,48 @@ def get_catalog(self) -> t.Optional[str]: | |||
| return self.catalog_name | |||
|
|
|||
|
|
|||
| class RisingwaveConnectionConfig(ConnectionConfig): | |||
There was a problem hiding this comment.
Same here. If there is nothing different in the connection properties between RisingWaveConnectionConfig and the PostgresConnectionConfig, why not just extend PostgresConnectionConfig and override the internal properties like type_ and _engine_adapter etc instead of copy+paste?
Unnecessary copy+paste is considered a code smell and doesn't meet the standards of this codebase
|
@lin0303-siyuan do you have plans to finish this? |
|
@tobymao Hi Toby, I have some issues last two weeks and plan to resume this week. |
Risingwave engine adapter is functioning well with simple model tests like create view, post statements...