Caideyipi commented on PR #17537: URL: https://github.com/apache/iotdb/pull/17537#issuecomment-4627409463
I noticed a few functional issues in the rewritten SQLAlchemy dialect: 1. DBAPI parameter substitution is not sufficient for SQLAlchemy Core DML. `Cursor.execute()` still turns SQLAlchemy's pyformat binds into SQL with plain Python `%` formatting: https://github.com/apache/iotdb/blob/ae5bc1b4edfc84f6d7af3fea38af7375bca98972/iotdb-client/client-py/iotdb/dbapi/Cursor.py#L108-L113 SQLAlchemy will compile normal Core statements such as `table.insert().values(region=asia, device_id=d001)` into SQL containing placeholders like `%(region)s` and pass the Python values separately. With the current code, string values become unquoted SQL identifiers (`VALUES (asia, d001, ...)`) rather than SQL string literals, so the new `insert().values(...)` / `delete().where(col == ...)` paths can fail before reaching the intended table-model behavior. This should either implement real bind handling in the DBAPI layer, or have the dialect/compiler safely render literals for strings, dates/timestamps, blobs, NULLs, booleans, etc. 2. `get_view_names()` is hard-coded to return an empty list: https://github.com/apache/iotdb/blob/ae5bc1b4edfc84f6d7af3fea38af7375bca98972/iotdb-client/client-py/iotdb/sqlalchemy/IoTDBDialect.py#L160-L161 The table model supports `SHOW VIEWS ...`, so SQLAlchemy Inspector/Superset will not be able to discover existing IoTDB views. It would be better to implement reflection via `SHOW VIEWS FROM <schema>` and add a test that creates a view and verifies it is returned. 3. `Text` currently compiles to `STRING`, while the README and reflection mapping say IoTDB `TEXT` maps to SQLAlchemy `Text`: https://github.com/apache/iotdb/blob/ae5bc1b4edfc84f6d7af3fea38af7375bca98972/iotdb-client/client-py/iotdb/sqlalchemy/IoTDBTypeCompiler.py#L56-L57 and https://github.com/apache/iotdb/blob/ae5bc1b4edfc84f6d7af3fea38af7375bca98972/iotdb-client/client-py/iotdb/sqlalchemy/IoTDBTypeCompiler.py#L92-L93 As a result, `Column(..., Text, iotdb_category=FIELD)` creates a `STRING` column instead of `TEXT`, and type round-tripping is inconsistent. `Text` should likely compile to `TEXT`, while `String`/VARCHAR-like types continue to compile to `STRING`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
