codeant-ai-for-open-source[bot] commented on code in PR #43000:
URL: https://github.com/apache/superset/pull/43000#discussion_r3748775033
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -419,6 +424,125 @@ def get_datatype(cls, type_code: str) -> str:
# keep it lowercase, as ClickHouse types aren't typical SHOUTCASE ANSI
SQL
return type_code
+ @classmethod
+ def df_to_sql(
+ cls,
+ database: Database,
+ table: Table,
+ df: Any,
+ to_sql_kwargs: dict[str, Any],
+ ) -> None:
+ """Upload a DataFrame to ClickHouse.
+
+ ClickHouse requires every table to declare a table engine, which the
+ `CREATE TABLE` that pandas' ``to_sql`` emits does not — so instead of
+ letting pandas create the table we create it ourselves with a MergeTree
+ engine and then append the rows.
+
+ The engine can be tuned per database through its ``extra`` JSON::
+
+ {"clickhouse_file_upload": {
+ "order_by": ["col1", "col2"],
+ "partition_by": "toYYYYMM(col1)",
+ "primary_key": "col1",
+ "settings": {"index_granularity": 8192}
+ }}
+
+ ``order_by`` defaults to ``tuple()`` (no sort key) so any upload works
+ without configuration.
+ """
+ # pylint: disable=import-outside-toplevel, import-error
+ import pandas as pd
+ from clickhouse_connect.cc_sqlalchemy.ddl.tableengine import MergeTree
+ from sqlalchemy import (
+ Column,
+ inspect,
+ MetaData,
+ Table as SqlaTable,
+ text,
+ types as sqltypes,
+ )
+
+ if_exists = to_sql_kwargs.get("if_exists", "fail")
+
+ if to_sql_kwargs.get("index"):
+ # Fold the index into columns so the table we create matches what
+ # gets inserted; we always append with index=False below.
+ df = df.reset_index()
+
+ config = (database.get_extra() or {}).get("clickhouse_file_upload", {})
+ order_by = config.get("order_by")
+ if order_by:
+ key_columns = set(
+ order_by if isinstance(order_by, (list, tuple)) else [order_by]
+ )
+ engine_kwargs: dict[str, Any] = {"order_by": order_by}
+ else:
+ # MergeTree still requires an ORDER BY; an empty tuple means
"none".
+ key_columns = set()
+ engine_kwargs = {"order_by": text("tuple()")}
+ for key in ("partition_by", "primary_key", "settings"):
+ if config.get(key):
+ engine_kwargs[key] = config[key]
+
+ def _column_type(dtype: Any) -> sqltypes.TypeEngine:
+ if pd.api.types.is_bool_dtype(dtype):
+ return sqltypes.Boolean()
+ if pd.api.types.is_integer_dtype(dtype):
+ return sqltypes.BigInteger()
+ if pd.api.types.is_float_dtype(dtype):
+ return sqltypes.Float()
+ if pd.api.types.is_datetime64_any_dtype(dtype):
+ return sqltypes.DateTime()
+ return sqltypes.String()
Review Comment:
**Suggestion:** The fallback maps every unsupported pandas dtype to
`String`, including object columns containing dates, decimals, UUIDs, bytes, or
other native values. This silently creates an incompatible or semantically
incorrect ClickHouse schema, so date and numeric operations can no longer work
after upload and some native values may fail during insertion. Type inference
should preserve the actual values or explicitly support the relevant
pandas/object dtypes. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Parquet uploads can lose decimal or UUID semantics.
- ⚠️ Date and numeric queries require string conversion.
- ⚠️ Native values may fail during ClickHouse insertion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=350ed4a7703c49a6bd58d1631d7f1eb4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=350ed4a7703c49a6bd58d1631d7f1eb4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/clickhouse.py
**Line:** 488:497
**Comment:**
*Type Error: The fallback maps every unsupported pandas dtype to
`String`, including object columns containing dates, decimals, UUIDs, bytes, or
other native values. This silently creates an incompatible or semantically
incorrect ClickHouse schema, so date and numeric operations can no longer work
after upload and some native values may fail during insertion. Type inference
should preserve the actual values or explicitly support the relevant
pandas/object dtypes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=f505ac22b6d22b02e2370dd5ca32b55d426ad1869ea9f36b9023eef6e6e13070&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=f505ac22b6d22b02e2370dd5ca32b55d426ad1869ea9f36b9023eef6e6e13070&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -419,6 +424,125 @@ def get_datatype(cls, type_code: str) -> str:
# keep it lowercase, as ClickHouse types aren't typical SHOUTCASE ANSI
SQL
return type_code
+ @classmethod
+ def df_to_sql(
+ cls,
+ database: Database,
+ table: Table,
+ df: Any,
+ to_sql_kwargs: dict[str, Any],
+ ) -> None:
+ """Upload a DataFrame to ClickHouse.
+
+ ClickHouse requires every table to declare a table engine, which the
+ `CREATE TABLE` that pandas' ``to_sql`` emits does not — so instead of
+ letting pandas create the table we create it ourselves with a MergeTree
+ engine and then append the rows.
+
+ The engine can be tuned per database through its ``extra`` JSON::
+
+ {"clickhouse_file_upload": {
+ "order_by": ["col1", "col2"],
+ "partition_by": "toYYYYMM(col1)",
+ "primary_key": "col1",
+ "settings": {"index_granularity": 8192}
+ }}
+
+ ``order_by`` defaults to ``tuple()`` (no sort key) so any upload works
+ without configuration.
+ """
+ # pylint: disable=import-outside-toplevel, import-error
+ import pandas as pd
+ from clickhouse_connect.cc_sqlalchemy.ddl.tableengine import MergeTree
+ from sqlalchemy import (
+ Column,
+ inspect,
+ MetaData,
+ Table as SqlaTable,
+ text,
+ types as sqltypes,
+ )
+
+ if_exists = to_sql_kwargs.get("if_exists", "fail")
+
+ if to_sql_kwargs.get("index"):
+ # Fold the index into columns so the table we create matches what
+ # gets inserted; we always append with index=False below.
+ df = df.reset_index()
+
Review Comment:
**Suggestion:** When `dataframe_index` is enabled, `reset_index()` creates
the index column using the DataFrame's existing index name or pandas' default
name, but removing `index_label` before `to_sql` discards the uploader's
requested column name. An upload with `index_label='foo'` therefore creates and
inserts a column such as `index` instead of `foo`, breaking the upload
contract. Preserve the requested label when folding the index into the
DataFrame. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Indexed uploads expose the wrong index column name.
- ⚠️ Downstream queries expecting configured labels fail.
- ⚠️ Affected CSV, Excel, and columnar upload workflows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=06253dda250c446281c6dc9b44bf0aeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=06253dda250c446281c6dc9b44bf0aeb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/clickhouse.py
**Line:** 468:472
**Comment:**
*Api Mismatch: When `dataframe_index` is enabled, `reset_index()`
creates the index column using the DataFrame's existing index name or pandas'
default name, but removing `index_label` before `to_sql` discards the
uploader's requested column name. An upload with `index_label='foo'` therefore
creates and inserts a column such as `index` instead of `foo`, breaking the
upload contract. Preserve the requested label when folding the index into the
DataFrame.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=3df971b9595c269580129935e937fb8fa58e2f1404fe693dd9e276197670c731&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=3df971b9595c269580129935e937fb8fa58e2f1404fe693dd9e276197670c731&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -419,6 +424,125 @@ def get_datatype(cls, type_code: str) -> str:
# keep it lowercase, as ClickHouse types aren't typical SHOUTCASE ANSI
SQL
return type_code
+ @classmethod
+ def df_to_sql(
+ cls,
+ database: Database,
+ table: Table,
+ df: Any,
+ to_sql_kwargs: dict[str, Any],
+ ) -> None:
+ """Upload a DataFrame to ClickHouse.
+
+ ClickHouse requires every table to declare a table engine, which the
+ `CREATE TABLE` that pandas' ``to_sql`` emits does not — so instead of
+ letting pandas create the table we create it ourselves with a MergeTree
+ engine and then append the rows.
+
+ The engine can be tuned per database through its ``extra`` JSON::
+
+ {"clickhouse_file_upload": {
+ "order_by": ["col1", "col2"],
+ "partition_by": "toYYYYMM(col1)",
+ "primary_key": "col1",
+ "settings": {"index_granularity": 8192}
+ }}
+
+ ``order_by`` defaults to ``tuple()`` (no sort key) so any upload works
+ without configuration.
+ """
+ # pylint: disable=import-outside-toplevel, import-error
+ import pandas as pd
+ from clickhouse_connect.cc_sqlalchemy.ddl.tableengine import MergeTree
+ from sqlalchemy import (
+ Column,
+ inspect,
+ MetaData,
+ Table as SqlaTable,
+ text,
+ types as sqltypes,
+ )
+
+ if_exists = to_sql_kwargs.get("if_exists", "fail")
+
+ if to_sql_kwargs.get("index"):
+ # Fold the index into columns so the table we create matches what
+ # gets inserted; we always append with index=False below.
+ df = df.reset_index()
+
+ config = (database.get_extra() or {}).get("clickhouse_file_upload", {})
+ order_by = config.get("order_by")
+ if order_by:
+ key_columns = set(
+ order_by if isinstance(order_by, (list, tuple)) else [order_by]
+ )
+ engine_kwargs: dict[str, Any] = {"order_by": order_by}
+ else:
+ # MergeTree still requires an ORDER BY; an empty tuple means
"none".
+ key_columns = set()
+ engine_kwargs = {"order_by": text("tuple()")}
+ for key in ("partition_by", "primary_key", "settings"):
+ if config.get(key):
+ engine_kwargs[key] = config[key]
+
+ def _column_type(dtype: Any) -> sqltypes.TypeEngine:
+ if pd.api.types.is_bool_dtype(dtype):
+ return sqltypes.Boolean()
+ if pd.api.types.is_integer_dtype(dtype):
+ return sqltypes.BigInteger()
+ if pd.api.types.is_float_dtype(dtype):
+ return sqltypes.Float()
+ if pd.api.types.is_datetime64_any_dtype(dtype):
+ return sqltypes.DateTime()
+ return sqltypes.String()
+
+ with cls.get_engine(
+ database, catalog=table.catalog, schema=table.schema
+ ) as engine:
+ has_table = inspect(engine).has_table(table.table,
schema=table.schema)
+ if has_table and if_exists == "fail":
+ # Raise ValueError so the uploader surfaces its friendly
+ # "table already exists" message (see UploadCommand).
+ raise ValueError(f"Table {table.table} already exists.")
+ if has_table and if_exists == "replace":
+ SqlaTable(table.table, MetaData(), schema=table.schema).drop(
+ engine, checkfirst=True
+ )
+ has_table = False
Review Comment:
**Suggestion:** Table existence checking, replacement, creation, and
insertion are separate non-atomic operations. Concurrent uploads can both
observe a missing table and race during `CREATE TABLE`, while any failure after
the `replace` drop leaves the original table absent. Because ClickHouse DDL is
not rolled back by the uploader's metadata transaction, this can cause failed
uploads or destructive data loss; use an atomic/staged replacement strategy and
coordinate concurrent creates. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Concurrent uploads can fail nondeterministically.
- ❌ Replace failures can leave tables deleted.
- ⚠️ Upload requests can lose previously available data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=067540f2f2a74e12a2b198efcba51e42&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=067540f2f2a74e12a2b198efcba51e42&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/clickhouse.py
**Line:** 499:511
**Comment:**
*Race Condition: Table existence checking, replacement, creation, and
insertion are separate non-atomic operations. Concurrent uploads can both
observe a missing table and race during `CREATE TABLE`, while any failure after
the `replace` drop leaves the original table absent. Because ClickHouse DDL is
not rolled back by the uploader's metadata transaction, this can cause failed
uploads or destructive data loss; use an atomic/staged replacement strategy and
coordinate concurrent creates.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=c5cdd72853c20b354ec20f053ab3c7f7ba5eb6406876302e6ab1e7fd8bd00889&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43000&comment_hash=c5cdd72853c20b354ec20f053ab3c7f7ba5eb6406876302e6ab1e7fd8bd00889&reaction=dislike'>👎</a>
--
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]
