joe-clickhouse commented on PR #43000:
URL: https://github.com/apache/superset/pull/43000#issuecomment-5247587297
Hi @s1ny1998 did you test this outside of the mocks in test? As written, I
wasn't able to upload any rows. Every upload failed before insertion.
Generally, I really like the idea of enabling uploads for the driver but in
running this `df_to_sql` against a live server I hit several problems. If I
were to steer this I'd make the following suggestions:
1. The spec inherits `supports_multivalues_insert = True`, so it passes
`method="multi"` to pandas. The clickhouse-connect SQLAlchemy dialect leaves
that capability false and pandas raises before sending anything to ClickHouse.
Please override `supports_multivalues_insert = False` and do not pass `method`.
I recommend bypassing pandas for the insert and calling
`engine.raw_connection().driver_connection.client.insert_df(...)`. This is
clickhouse-connect's native bulk path and follows the BigQuery spec's precedent
of using the driver's native loader. It avoids pandas' per-row dictionaries and
1,000-row batches.
2. Once insertion works, the generated schema seems to change data. In my
live test:
- None in a text column became an empty string
- 0.1 became 0.10000000149011612
- 1e40 became infinity
- a 1965 timestamp became the Unix epoch
- microseconds were discarded
`nullable=True` does not produce `Nullable(...)` for generic SQLAlchemy
types in this dialect. Generic `Float()` also becomes ClickHouse `Float32`,
while generic `DateTime()` has second precision and the `DateTime` range.
Please use clickhouse-connect types directly: Nullable(String),
Nullable(Float64), Nullable(DateTime64(6)), Nullable(Bool), and Nullable(Int64)
or Nullable(UInt64). The code already depends on this package for MergeTree.
3. Superset currently does not classify Nullable(Float64) as numeric.
`get_column_spec("Nullable(Float64)")` returns None because the Float mapping
is not wrapper tolerant. Please add a `.*Float.*` numeric mapping so uploaded
floats are aggregatable in Explore.
4. Pass `MergeTree(...)` as a positional Table argument.
`clickhouse_engine=` raises `ArgumentError` when clickhouse-sqlalchemy is also
installed because that package owns the clickhouse keyword namespace. The
keyword also requires newer clickhouse-connect versions while this spec still
advertises >=0.13.0. Positional attachment works across the supported range.
5. I would omit the database extra configuration from the first version.
ORDER BY tuple() does seem like the right default for ad hoc upload tables. The
configuration is hidden, applies to every upload for that database, and has
invalid cases. For example, `primary_key` without `order_by` produces DDL the
server rejects. Users who need sorting or partitioning can create the table in
SQL Lab and upload with append.
6. Type inference should prefer correctness over guessing. At minimum, I
think it's worth to distinguish unsigned integers. Pandas reads
`9223372036854775808` as `uint64`, which cannot fit in `Int64`. When an exact
numeric representation cannot be inferred, I think `String` is safer than
silent rounding.
7. Please test the real dialect rather than replacing SQLAlchemy Table,
MergeTree, and `DataFrame.to_sql` with mocks. Compile the actual CREATE TABLE
and assert the emitted ClickHouse types. Add a direct assertion that
Nullable(Float64) is classified as numeric. These tests need no server and
cover the failures above.
I am filing clickhouse-connect follow-ups for generic type mapping and
nullability. For now, its native types are the reliable route. Happy to review
the next revision and help answer wny other clickhouse or clickhouse-connect
specific questions along the way. Thanks again!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
