Repository navigation
fix(unparser): keep a qualifier that an UNNEST or FLATTEN alias, a schema-qualified table or a struct field path binds (refs spiceai/spiceai#13442) - #246
Merged
Conversation
…type (refs spiceai/spiceai#14491) A Date32 literal is rendered as CAST('YYYY-MM-DD' AS DATE) whatever the dialect, while an Expr::Cast to Date32 takes Dialect::date32_cast_dtype. SQLite gives the type name DATE numeric affinity, so the literal form is the integer 1994 and a YYYY-MM-DD text column compares greater than it, which turns a date range filter pushed into a SQLite scan into one that matches no row. The literal now takes the same target type the cast does.
ListingTable materializes metadata columns (_last_modified, _size, _location) from each object's ObjectMeta, but pruning only used partition columns. A predicate such as `_last_modified > <watermark>` fell through to a row-level FilterExec, so every file was opened and read before its rows were dropped. For a compressed row format (e.g. jsonl.gz) this decompressed the whole object on every scan. Prune the listing by these predicates before any file is opened: - `pruned_partition_list_with_metadata` filters the listed ObjectMeta set by evaluating the metadata-only predicate against each object. The existing `pruned_partition_list` delegates to it with empty metadata arguments, so its signature and behavior are unchanged. - `filter_by_metadata` evaluates the predicate via create_physical_expr against a one-row batch built from MetadataColumn::to_scalar_value, giving correct >, >=, <, <=, =, BETWEEN, IN, LIKE, cast and NULL semantics. A NULL (unknown) result prunes the file, matching SQL WHERE semantics. - `supports_filters_pushdown` reports a metadata-only conjunct as Exact, so the redundant FilterExec is dropped. A mixed metadata+data predicate, or one under OR/NOT, stays Inexact and keeps a residual filter (correct, not optimal). Metadata pruning is orthogonal to partition pruning and applies to both partitioned and unpartitioned tables, and to all file formats. Reproduces spiceai/spiceai#14264.
A caller that obtains an ObjectMeta from a HEAD on a known key (rather than a listing) can apply the identical metadata-column prune before opening the file, and report the same predicates as Exact.
Compile the metadata predicate once per listing instead of per file (MetadataPredicate), threading the caller's session ExecutionProps through so filters using session variables still evaluate correctly. Deduplicate metadata-column-name extraction between scan_with_args and supports_filters_pushdown, and add test coverage: unit-level predicate tests, a listing-helper integration test, and a ListingTable plan-level test asserting the Exact pushdown marking and scan wiring actually prune file groups and residual filters as expected. Also documents the Exact-pushdown contract so a TableProvider wrapping ListingTable with its own scan path doesn't inherit it without enforcing it.
fix(unparser): spell a Date32 literal's cast with the dialect's date type (refs spiceai/spiceai#14491)
…FROM makes visible (refs spiceai/spiceai#13442) The sweep that strips a column qualifier no FROM relation answers to decided "no relation" from too little: it collected only Table and Derived factors, so an UNNEST or LATERAL FLATTEN alias was invisible and `u.c1` lost its qualifier (ambiguous over two FLATTENs); it recorded a bare multi-part table as its full spelling while a column is emitted with the table part alone, so every column over a schema-qualified table was stripped; it compared every part but the last, so a struct field path `test."metadata".product` was judged by `test."metadata"` and cut to `product`; and a reference left with one part stayed a one-element CompoundIdentifier, which a planner handed the AST refuses. VisibleScope now records every alias-bearing TableFactor, a bare table by its last name part (and its full name for a dialect that writes every part), compares identifier values, and reads an un-aliased derived table's output names so a field path rooted at one is not mistaken for a dangling qualifier. Stripping removes only the qualifier and produces an Identifier when one part remains. The select-level sort sweep is written back; it previously mutated a clone. Twenty of the branch's 22 failing datafusion-sql unparser tests pinned the output this damaged. The remaining expectation pins the derived-table sort that fork PR #191 deliberately hoists; its snapshot is updated. Three upstream expectations are updated to the fork's deliberate output, each with a comment.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved qualifier disambiguation can emit invalid SQL or change field references, requiring human review.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Updates DataFusion’s SQL unparser to judge dangling qualifiers against relations visible in the emitted FROM.
Changes:
- Adds visibility tracking for tables, derived tables, UNNEST, and FLATTEN.
- Preserves field paths, normalizes stripped identifiers, and writes back select-level sorts.
- Updates unit tests and SQL expectations.
| File | Description |
|---|---|
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_reference_an_outer_body_answers_to.snap | Retains visible table qualifiers. |
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_reference_an_inner_body_answers_to.snap | Retains visible table qualifiers. |
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_nested_reference_the_enclosing_body_answers_to.snap | Updates nested EXISTS output. |
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_nested_reference_a_sibling_branch_names.snap | Updates sibling-branch qualifiers. |
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_nested_reference_a_joined_body_names.snap | Preserves joined-table qualifiers. |
| datafusion/sql/tests/cases/snapshots/sql_integration__cases__plan_to_sql__unparse_left_semi_join_keeps_nested_outer_reference_neither_side_answers_to.snap | Retains outer table qualifiers. |
| datafusion/sql/tests/cases/plan_to_sql.rs | Updates three SQL expectations and explanatory comments. |
| datafusion/sql/src/unparser/rewrite.rs | Adds visible-scope tracking, qualifier cleanup, and tests. |
| datafusion/sql/src/unparser/plan.rs | Applies scope-aware cleanup and saves rewritten sorts. |
| datafusion/sql/src/unparser/ast.rs | Supplies relation visibility from builders. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…so the qualifier sweep never guesses (refs spiceai/spiceai#13442) Copilot's review found two shapes where the sweep still misjudged a qualifier, both because get_field_to_sql flattened a column's qualifier, name and field path into one CompoundIdentifier: a derived table exposing a column named like the relation it encloses (t(t, id)) kept a dangling `t.id`, and under a dialect that writes every qualifier part a dangling `catalog.schema.table.payload.item` lost `payload` with the qualifier. A field path is now emitted as CompoundFieldAccess with the column reference as its root, the shape the scalar-function root already used; the text is unchanged. A CompoundIdentifier is therefore a qualifier plus one column name, the sweep judges only that, and the derived-output-name heuristic is gone. The sweep also covers an aliased projection item, which it skipped. One expectation pinned the dangling aliased form and is updated.
…fier exactly, and replan a compound field root (refs spiceai/spiceai#13442) Four shapes from the second review round: - A correlated reference binds in the enclosing query, so the sweep now collects the qualifiers of the SELECT's own OuterReferenceColumns and treats them as visible. Aliased items had just joined the sweep, and an outer reference under an alias would have lost its qualifier; unnamed ones had been losing it before this branch. - A CompoundIdentifier is a qualifier plus one column, so a visible name must match the whole qualifier, not a prefix: an alias `d` does not make `d.t.id` bind when `d.t` is the enclosed table. - An unquoted `@root.name` is a scalar variable, not a qualified column, by the planner's own rule, and is left alone. - The planner's access-chain extraction accepted only a plain identifier root before folding leading Dot identifiers into one qualified reference; a CompoundIdentifier root fell through and its trailing Dot was rejected when the emitted AST was planned directly. It folds a compound root too.
…llow-listing its qualifier (refs spiceai/spiceai#13442) Adding an outer reference's qualifier to the SELECT-wide visible scope was a judgement by spelling: a local `t.id` read through an un-aliased derived table kept its qualifier whenever the enclosing query also aliased `t`, and then bound to the enclosing row. The plan walk that gathered those qualifiers also reached subquery inputs and lateral children. An OuterReferenceColumn is now emitted through mark_outer_reference, which gives each of its identifiers a sentinel Span: the one Ident field that neither prints nor takes part in equality or hashing, so the statement and every AST comparison are unchanged. The sweep leaves a marked reference alone and judges an unmarked one against the FROM as before; the plan walk is gone.
Author
Nothing technical is left — this needs one human approvalRead back just now on head
What a reviewer is being asked to confirm: that judging a dangling qualifier against every relation the @krinart is already the requested reviewer. Holding here; a later |
Author
The blocker has changed: this is no longer "needs one approval"My earlier note on this PR said "No conflict. Current state, read back just nowThe
|
…g the base's visibility model The branch was cut from spiceai-54 and carried that branch's metadata-column listing pruning and Date32 literal commits; spiceai-55 has both as its own ports (#240, #242), so datafusion/catalog-listing and datafusion/core resolve to spiceai-55's version. spiceai-55 also answers "which names does this FROM make visible" with collect_relation_idents. This branch's VisibleScope / strip_dangling_qualifier redesign of the same sweep is dropped, together with the rest of its production changes, so the unparser here is exactly spiceai-55's. The branch's tests are kept on top of the base's: the five plan-level tests, the qualified struct field path in roundtrip_statement, the three upstream expectations the fork deliberately changed (roundtrip_statement_with_dialect_5, test_table_scan_pushdown, the hoisted sort), and the six EXISTS snapshots. test_unparse_extension_to_sql keeps the base's expectation. The field-path test now expects the alias spiceai-55 adds to keep the plan's output name. At this commit those tests fail on the base's unparser; the next commit is the fix.
…LATTEN, a schema-qualified table or a struct field path (refs spiceai/spiceai#13442) The dangling-qualifier sweep strips a column qualifier no FROM relation answers to. Four gaps in what it treated as visible made it strip qualifiers that do bind, and they account for the 19 datafusion-sql tests that still fail on spiceai-55 for spiceai/spiceai#13442: - An `UNNEST(...) AS u` or `LATERAL FLATTEN(...) AS u` relation was not collected, so `u.c1` lost its qualifier (two of them sharing a column name gave `SELECT c1, c1`, which DuckDB rejects as ambiguous). collect_relation_idents and RelationBuilder::get_alias now report both. - A bare multi-part table was collected only by its full name, while a column over it is qualified by the last part (`"table".id` over `"schema"."table"`), so `SELECT a.v, b.v FROM s.a JOIN s.b` became the ambiguous `SELECT v, v`. The last part is collected too. - A struct field path was one flat CompoundIdentifier, judged by everything but its last part: `test."metadata".product` was cut to `product`, a column that does not exist. get_field_to_sql now emits CompoundFieldAccess with the column reference as its root (the text is unchanged), the sweep judges that root, and the planner folds a compound-identifier root of an access chain the way it already folded a plain one, so the emitted AST still plans. - A reference stripped to one part was left a one-element CompoundIdentifier, which the planner refuses ("Not a compound identifier") when handed the AST. It becomes a plain Identifier. The visibility model is spiceai-55's collect_relation_idents unchanged in shape; this only widens what it collects.
…correlated subscript keeps its qualifier (refs spiceai/spiceai#13442) The previous commit judged the root of every CompoundFieldAccess against the FROM. That also reached an array subscript whose root is a correlated reference: `SELECT (SELECT o.array_col[1] FROM unnest_table AS i LIMIT 1) FROM unnest_table AS o` was emitted with `array_col[1]`, which reads the subquery's own row (DuckDB returns 10, 10 instead of 10, 20 over rows [10] and [20]). spiceai-55 never swept a field access, so this was a regression of that commit. A field access is left alone again; a struct field path no longer needs the sweep to be read correctly, because its column's qualifier now sits in the access's root rather than at the front of a flat identifier.
…anged (refs spiceai/spiceai#13442) With field accesses out of the sweep again, the only behaviour left in the remove_dangling_qualifier helper was turning a reference stripped to one part into a plain Identifier. No test this PR targets needs it: the one roundtrip_statement entry it was added for (`SELECT s.a FROM (SELECT {a:1, b:2} AS s)`) was a struct field path, now emitted as a CompoundFieldAccess the sweep does not touch. It also would have made a stripped correlated reference plannable from the AST, where the base fails loudly. The helper is removed and the sweep sites are spiceai-55's again, so the change to the sweep is only what it treats as visible.
grokspice
changed the title
Author
Rebuilt on spiceai-55's visibility modelThe hold from the earlier escalation is released: the conflict is resolved and the branch is rebuilt as the smallest fix on top of spiceai-55. Head is What changed, and why
Red → green, same commandEach run compiled all 15 workspace crates from its own tree: The 22 base failures are the issue's list (names in the PR body). None of them fail on the branch, and nothing that passes on the base fails here. Executed in DuckDB and SQLite, the base's output for three of these shapes is rejected ( The adversarial review (codex) caught one regression in an intermediate revision.
Diff vs spiceai-55Left out deliberately (pre-existing on spiceai-55, same output before and after)Each of these is reproduced and described in the PR body:
The first two need the sweep to know where a reference came from, which the base's model does not carry. |
…eep, but never a subscript's (refs spiceai/spiceai#13442) With a field path emitted as CompoundFieldAccess, the sweep (which matched only a bare CompoundIdentifier) no longer looked at it at all. Where no rescoping reaches the derived table, the qualifier then dangled: `Projection(get_field(t.metadata, 'product')) -> Limit(1) -> TableScan(t)` emitted `SELECT t."metadata".product FROM (SELECT * FROM t LIMIT 1)`. spiceai-55 emitted `SELECT product FROM ...` there, a column that does not exist, so neither form ran. The sweep now also judges the column a path of `.field` steps is rooted at, and keeps the path: `SELECT "metadata".product FROM (SELECT * FROM t LIMIT 1)`. That shape comes only from get_field, whose root is always a column of the query's own relations (a correlated column is refused there), so it cannot strip a correlated reference. A subscript stays untouched: its root can be correlated, as test_correlated_array_subscript_keeps_its_qualifier pins.
This was referenced Oct 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
datafusion-sqlunparser tests). The spiceai issue closes with the pin bump that brings this fix across.Rationale for this change
spiceai-55still fails the same 22datafusion-sqltests that spiceai/spiceai#13442 lists forspiceai-54(measured atddd5c8ce6, below). Nineteen are the fork's dangling-qualifier sweep stripping a qualifier that does bind. The other three pin upstream output that the fork changed on purpose.The sweep strips a column qualifier that no
FROMrelation answers to.spiceai-55decides which names are visible withcollect_relation_idents. This PR keeps that model and only widens what it collects. Earlier revisions of this branch replaced the sweep with aVisibleScopeobject; that redesign is dropped (see "History" below).What the base gets wrong. Each item shows the SQL the base emits and what a real engine does with it:
UNNEST(…) AS uorLATERAL FLATTEN(…) AS ualias is not collected.u.c1loses its qualifier. Two of them sharing a column name giveSELECT c1, c1 FROM UNNEST([1, 2]) AS a (c1) CROSS JOIN UNNEST([3]) AS b (c1), and DuckDB answersAmbiguous reference to column name "c1"."table".idover"schema"."table"), so a join of two such tables becomesSELECT v, v FROM s.a INNER JOIN s.b ON a.id = b.id. DuckDB and SQLite both reject that as ambiguous.CompoundIdentifier, judged by everything but its last part.test."metadata".productis cut toproduct(DuckDB:Referenced column "product" not found).SELECT s.a FROM (SELECT {a:1, b:2} AS s)becomesSELECT a FROM ….What changes are included in this PR?
plan.rs:collect_relation_identsalso collects the alias of anUNNESTand of a table function (LATERAL FLATTEN). A bare table contributes its full name and, for a multi-part name, its last part (collect_table_name_idents). This applies to the main relation and to joined ones. The projection, queryORDER BYand select-level sort sweeps callremove_dangling_qualifier.rewrite.rs:remove_dangling_qualifierapplies spiceai-55'sremove_dangling_identifiersto a bare column reference, as before. It also applies it to the column a path of.fieldsteps is rooted at, keeping the path. That shape only comes fromget_field, which refuses a correlated column. A subscript (o.a[1]) is left alone, because its root can be a correlated reference.ast.rs:RelationBuilder::get_aliasreports the alias of anUnnestorFlattenrelation (it returnedNone). A crate-privatetable_name()exposes a bare table'sObjectName.expr.rs:get_field_to_sqlemits a field path on a column asCompoundFieldAccess { root: <column reference>, access_chain }. That is the shape it already used for a scalar-function root, and the text is unchanged. The sweep can now tell the column's qualifier from the fields after it, instead of judgingtest."metadata"as if it were a qualifier.expr/mod.rs(planner):extract_root_and_access_chainfolds aCompoundIdentifierroot the same way as anIdentifierroot. Without that, the emitted AST foru.struct_col.field1fails when handed to the planner directly (theroundtrip_statementpath) withNotImplemented("Dot access not supported for non-string expr: Identifier(… "field1" …)").test_correlated_array_subscript_keeps_its_qualifier, covering the projection and theORDER BY.test_field_path_over_a_limit_input_is_judged_by_its_column. It covers a field path over aLIMITinput that no rescoping reaches, and one under a visible alias over aLIMIT; spiceai-55 cut the latter tofield1.test_remove_dangling_qualifier(unit).roundtrip_statement.roundtrip_statement_with_dialect_5,test_table_scan_pushdown, andtest_sort_with_scalar_fn_and_push_down_fetch, whose sort fork PR fix(unparser): keep ORDER BY out of a derived table when the sort key is computed #191 hoists.unparse_left_semi_join_keeps_*snapshots now keep"t"."c"overFROM "public"."t"instead of the bare"c".SELECT "metadata".product AS "t.metadata[product]" FROM (SELECT t."metadata" FROM t). In DuckDB the alias is what makes the result columnt.metadata[product], the plan's own name; without it the column is namedproduct.Are these changes tested?
The same command was run on the base, on this branch's tests over the base's unparser, and on this branch. Each run compiled all 15 workspace crates from its own tree:
The 22 failing on the base, by name, are all in
cases::plan_to_sql:roundtrip_statement,roundtrip_statement_with_dialect_{5,34,36,37,42}snowflake_flatten_{cross_join_unnest_table_column,from_unnest_with_table_alias,implicit_from,multiple_unnest_cross_join,select_unnest_plus_literal,sort_between_projection_and_unnest,string_array,unnest_from_subselect}snowflake_unnest_to_lateral_flatten_{cross_join_inline,simple,with_cross_join}test_sort_with_scalar_fn_and_push_down_fetch,test_struct_expr,test_table_references_in_plan_to_sql_{1,2},test_table_scan_pushdownThe branch fails none of them, and no test that passes on the base fails here. The extra 3 red at
71d39305c(25 rather than 22) are the 6 EXISTS snapshots, minus the 3 expectation updates.The emitted SQL was executed in DuckDB 1.4.4 and SQLite 3.50.4. Base outputs are from
e8d8abae9, the base tip when they were captured:test_struct_expr)SELECT product FROM (…) AS test WHERE …→ DuckDB:Referenced column "product" not foundSELECT test."metadata".product FROM (…) AS test WHERE …→ 1 rowUNNESTs, same column nameSELECT c1, c1 FROM UNNEST([1, 2]) AS a (c1) CROSS JOIN UNNEST([3]) AS b (c1)→ DuckDB: ambiguousc1SELECT a.c1, b.c1 …→(1, 3), (2, 3)s.aands.bSELECT v, v FROM s.a INNER JOIN s.b ON a.id = b.id→ DuckDB and SQLite: ambiguousvSELECT a.v, b.v …→('from_a', 'from_b')in bothA correlated array subscript was found by the adversarial review of an intermediate revision (
06692b5f3), which swept the root of every field access, subscripts included. The regression test fails on that revision and passes here, same command:Copilot's review of
65f7f8e95found a field path over aLIMITinput left with a hidden qualifier. It is reproduced, fixed in55a705a06, and the thread is answered. Same command:spiceai-55 emits
SELECT product FROM (SELECT * FROM t LIMIT 1)for that plan, a column that does not exist.Over rows
[10]and[20], DuckDB returns10, 20for the kept form and10, 10for the stripped one. spiceai-55 keeps the qualifier, so the final diff does not change this shape.cargo fmt --all -- --checkexits 0.cargo clippy -p datafusion-sql --all-targets -- -D warningsexits 0.Not in this PR (pre-existing on spiceai-55, unchanged here)
Each of these emits the same text on the base and on this branch:
SELECT (SELECT o.j1_id FROM j1 AS i LIMIT 1) FROM j1 AS ois emitted asSELECT (SELECT j1_id FROM j1 AS i LIMIT 1) FROM j1 AS o, which DuckDB answers1, 1instead of1, 2. Fixing it needs the sweep to know where a reference came from, which the base's model does not carry.test_unparse_extension_to_sqlstill pinsSELECT j1.j1_id AS user_id FROM (SELECT j1.j1_id, j1.j1_string FROM j1), which SQLite (no such column: j1.j1_id) and DuckDB both reject. Sweeping aliased items was tried. It strips the correlatedj1.thattest_correlated_reference_keeps_its_qualifier_under_an_aliaspins (SELECT (SELECT j1_id AS x FROM j2 LIMIT 1) FROM j1), so it needs the same provenance work.CompoundIdentifier. The printed SQL is unaffected. A planner handed the AST directly refuses it:select j1_id from (select j1_id from j1 limit 10)givesNot a compound identifier. Converting it to anIdentifierwould also make a stripped correlated reference plannable where the base fails loudly, so it is left as it is.WHEREdoes not plan from the AST.test."metadata".product."name"fails withDot access not supported for non-string expr.Are there any user-facing changes?
Federated SQL keeps the qualifier in the shapes above where the base removed one that binds. The emitted text of every other statement is unchanged. The one public API change:
RelationBuilder::get_aliasnow returns the alias of anUNNESTorFLATTENrelation instead ofNone.History
Earlier revisions of this branch were cut from
spiceai-54. They replaced the sweep with aVisibleScope/strip_dangling_qualifierdesign and also swept aliased items, carrying a correlated-reference marker for that. Afterspiceai-55landedcollect_relation_idents, that design was dropped in favour of the base's. The branch'sspiceai-54listing and Date32 commits resolve to spiceai-55's own ports (#240, #242), andgit diff origin/spiceai-55 -- datafusion/catalog-listing datafusion/coreis empty.Review gate
55a705a06,codexexited 1 twice. The first run "did not return valid structured JSON"; the second reported "Your workspace is out of credits".grokis not a permitted fallback for a Grok-authored diff. The engine did review the earlier revisions:review-mvbm7mhz-032sb8on65f7f8e95approved, with no material findings. The only change since is the.field-path sweep in55a705a06, which is covered by the red/green test above but was not reviewed by an engine.review-mvbl88qb-3h59c5on06692b5f3returned one high finding: a correlated array subscript lost its qualifier. Reproduced, fixed ind7e39c62d, with a regression test.review-mvblpfno-2pbbcxond7e39c62dreturned one high finding: a plain correlated reference loses its qualifier. Reproduced as pre-existing on spiceai-55 with identical output, and listed above as not in this PR. The one-partIdentifierconversion that would have made it plannable from the AST was removed in65f7f8e95./security-review: skipped — the command reviews the session's own checkout, which is a different clone of this repository. Instead, the diff was read for security issues in this session. It adds no input handling: identifiers keep the unparser's quoting, and the planner change routes a compound root through the existingsql_compound_identifier_to_expr. No findings./simplify: not run. The diff was cut down by hand to the collection, emission and.field-path sweep changes.