cetra3 commented on issue #25459: URL: https://github.com/apache/datafusion/issues/25459#issuecomment-6463604945
I have done some exploration with a coding agent to deal with this issue more fundamentally rather than playing whackamole: #26842. The core of this is adding ids to columns. At the core of it, using names to refer to columns causes a whole raft of bugs. A column and a cast of it (`t.a` and `CAST(t.a AS ...)`, which is also named `t.a`), two scans of the same table, two references to a CTE, or a column and an extracted leaf expression can all end up with the same name. When a rule moves or rebuilds a node and looks columns up by name, it finds the wrong one, or none. Things like `__datafusion_extracted_N` (plus the reserved prefix, and skipping past user aliases that use it) feel like a bit of a smell if we have to do this just to keep names unique. This draft PR passes all tests, and fixes a plethora of issues. These are **12 open issues** with one mechanism, including wrong results and bugs that have been open since 2022: | Issue | Opened | Symptom on `main` | |---|---|---| | #26692 | 2026-10 | **wrong results**: decorrelation loses correlated columns when a subquery has two equal scans | | #26621 | 2026-10 | `No field named t.a`, or a panic (`GROUP BY CAST(a AS VARCHAR), a` with CSE) | | #26573 | 2026-10 | `No field named x.a` (`= ANY (subquery)` in a join over an aliased table) | | #25837 | 2026-09 | schema error in a LATERAL subquery over a derived table alias | | #26622 | 2026-10 | internal schema error for `x > ALL (subquery)` in the SELECT list | | #26033 | 2026-10 | `No field named __correlated_sq_1.y` (correlated `IN` with a cast) | | #26221 | 2026-10 | `duplicate unqualified field name` for `sum(x) AS a, sum(CAST(x ...)) AS b` (the unaliased form is still rejected) | | #25447 | 2026-09 | leaf extraction computes the same expression twice | | #3353 | 2022-09 | `approx_distinct(c9)` and `approx_distinct(CAST(c9 ...))` in one query fail | | #4478 | 2022-12 | `SELECT a, CAST(a AS INT)` fails to project | | #6543 | 2023-06 | two columns with the same alias are rejected | | #13558 | 2024-11 | `SELECT CAST(c AS TEXT) ... ORDER BY c` fails with a duplicate expression error | Most of these are each patched in their own open PR, one rule at a time (#26702, #26596, #26044, #26699, #26638, #26558, #26376, #26350). Here they all fall out of the same change. It also covers the class of #26619, which #26783 just fixed in `PushDownFilter`. And because names no longer need to be unique, `__datafusion_extracted_N`, `__common_expr_N`, their alias generators and the reserved prefix all go away. In the draft, every column of a logical plan gets a `ColumnId`, the way Spark (`ExprId`), DuckDB (`ColumnBinding`) and Trino (symbols) do it: - The id is created where the column is defined (a scan, an alias, a union) and stored with the definition, so it survives rewrites. No pass assigns or repairs ids afterwards. - References get the id when their names are resolved, and lookups use the id first. - CSE and leaf extraction name the columns they define after their expressions and reference them by id. Extraction projections are marked by an alias kind instead of a name prefix. - A validator checks after every rule that references are consistent with the columns they read. Across the whole sqllogictest suite, no reference resolves to a different column by id than by name. It is a draft to discuss the direction, not something to merge as is. The open points are in the PR description: - planning cost isn't measured yet; - some code still builds references by name and relies on a name fallback; - plans decoded from protobuf or Substrait, and extension nodes, don't carry ids; - EXPLAIN output changes. The targeted fixes can land first. If this is a direction people want, the PR description has a staged plan for landing it in smaller pieces. -- 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]
