Fix: Ensure correct datatypes are fetched for RisingWave dialect #4903
Conversation
|
|
| exp.column("name", db="rw_relations").eq(table.alias_or_name), | ||
| exp.column("name", db="rw_columns").neq("_row_id"), | ||
| exp.column("name", db="rw_columns").neq("_rw_timestamp"), |
There was a problem hiding this comment.
why are we setting db here instead of table?
|
|
||
| if table.args.get("db"): | ||
| sql = sql.where( | ||
| exp.column("name", db="rw_schemas", quoted=False).eq(table.args["db"].name) |
There was a problem hiding this comment.
The above is directly copied from
I was following the example set in the BasePostgresAdapter by using db
There was a problem hiding this comment.
Notice how we only access it for table, though. For columns, i.e. objects constructed via column, a reference like a.b produces a b for this and a for table. The db part would come before a, and similarly the catalog part would come before the db part.
There was a problem hiding this comment.
Thank you for the clarification. I will update next week to use table over db.
I guess I was just blindly following the existing code, but did consider whether it was due to some interaction with Postgres as a single catalog engine. It seemed to work so I didn't question it too deeply.
| @@ -15,6 +15,43 @@ def adapter(make_mocked_engine_adapter): | |||
| return adapter | |||
|
|
|||
|
|
|||
| def test_columns(adapter: t.Callable): | |||
There was a problem hiding this comment.
Let's include an integration test for this please, containing the problematic case you described in the description. You can create a test_integration_risingwave, similar to other integration testing modules.
There was a problem hiding this comment.
I'll try and get started on this next week.
Prior to this PR, the RisingWave dialect used the
BasePostgressAdapterimplementation ofdef columns(..)to fetch column names and data-types.However the PG catalog tables in RisingWave doesn't support e.g.
structtypes instead returning???as datatype. This PR updates the RisingWave adapter to use the RisingWave catalogs instead to get correct datatypes.