codeant-ai-for-open-source[bot] commented on code in PR #41946:
URL: https://github.com/apache/superset/pull/41946#discussion_r3562906886
##########
superset/connectors/sqla/models.py:
##########
@@ -2017,12 +2022,14 @@ def fetch_metadata(self) -> MetadataResult:
if col.get("comment"):
new_column.description = col["comment"]
db_engine_spec.alter_new_orm_column(new_column)
+ if expression:
+ new_column.expression = expression
else:
new_column = old_column
if new_column.type != col["type"]:
results.modified.append(col["column_name"])
new_column.type = col["type"]
- new_column.expression = ""
+ new_column.expression = expression
Review Comment:
**Suggestion:** Setting `expression` on existing synced columns makes those
columns truthy for `col.expression`, but `fetch_metadata` still re-adds all old
truthy-expression columns at the end. On a second metadata sync, expanded
nested columns (like Trino `metadata.uuid`) will be appended once in the main
loop and again in the “add back calculated columns” step, producing duplicate
column entries. Only re-add columns that were not matched by source metadata
(for example, from the remaining `old_columns_by_name` values) instead of all
`old_columns` with expressions. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset refresh creates duplicate columns for nested ROW fields.
- ⚠️ Dataset API GET /api/v1/dataset/<id> returns duplicate columns.
- ⚠️ Drill info API includes duplicated column metadata entries.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Trino database with `"schema_options": {"expand_rows": True}`
so that
`TrinoEngineSpec.get_columns()` (superset/db_engine_specs/trino.py:640-665)
returns
expanded nested `ROW` fields with dotted `column_name` and per-segment-quoted
`expression`.
2. Create a SQLA dataset pointing at a Trino table with a nested ROW column
(e.g.
`metadata` with subfield `uuid`), and run a first metadata sync via
`RefreshDatasetCommand.run()` (superset/commands/dataset/refresh.py:6-27)
which calls
`SqlaTable.fetch_metadata()` (superset/connectors/sqla/models.py:31-125),
producing
`TableColumn` entries including `"metadata.uuid"` with its `expression` set
from
`col["expression"]` at lines 77-103.
3. Trigger a second "Sync columns from source" on the same dataset (again
executing
`RefreshDatasetCommand.run()` → `fetch_metadata()`); now `old_columns`
(models.py:54-62)
contains the previously persisted physical columns, including
`"metadata.uuid"` with a
non-empty `expression`, and the main loop (models.py:77-109) matches this
existing column
via `old_column = old_columns_by_name.pop(col["column_name"], None)` and
reuses it as
`new_column`, setting `new_column.expression = expression` at line 103 and
appending it to
`columns`.
4. After the loop, `fetch_metadata()` executes the "add back calculated
(virtual) columns"
step `columns.extend([col for col in old_columns if col.expression])` at
line 113, which
iterates over the original `old_columns` list (not the pruned
`old_columns_by_name`) and
re-adds every column with a truthy `expression` — including the
already-synced physical
`"metadata.uuid"` column — resulting in duplicate `TableColumn` entries with
the same
`column_name` in `self.columns`, which are then exposed via dataset APIs
(e.g.
superset/datasets/api.py `get()` and `get_drill_info()` around lines
1500-1577).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=34e03271b20e418d9a46446229ad88f5&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=34e03271b20e418d9a46446229ad88f5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 2012:2032
**Comment:**
*Logic Error: Setting `expression` on existing synced columns makes
those columns truthy for `col.expression`, but `fetch_metadata` still re-adds
all old truthy-expression columns at the end. On a second metadata sync,
expanded nested columns (like Trino `metadata.uuid`) will be appended once in
the main loop and again in the “add back calculated columns” step, producing
duplicate column entries. Only re-add columns that were not matched by source
metadata (for example, from the remaining `old_columns_by_name` values) instead
of all `old_columns` with expressions.
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%2F41946&comment_hash=2586d085d5cd1608db8bf4e4aa41fd2d7f06cee06542aaf9b46e703cfca907c0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41946&comment_hash=2586d085d5cd1608db8bf4e4aa41fd2d7f06cee06542aaf9b46e703cfca907c0&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]
