fix(unparser): keep ORDER BY out of a derived table when the sort key is computed - #191
Open
claudespice wants to merge 1 commit into
Open
fix(unparser): keep ORDER BY out of a derived table when the sort key is computed#191claudespice wants to merge 1 commit into
claudespice wants to merge 1 commit into
Conversation
… is computed `rewrite_plan_for_sort_on_non_projected_fields` hoists a `Sort` that sits between two `Projection`s up to the top of the plan so the emitted statement carries the ORDER BY itself. It only did so when the inner `Projection`'s outputs were exactly the outer `Projection`'s expressions plus the sort keys, compared as strings. A sort key that is an *expression over* a projected column -- `ORDER BY age + 1` where the inner `Projection` exposes `age` -- never matched, so the rewrite bailed out and `select_to_sql_recursively` emitted the `Sort` as a derived table: SELECT id FROM (SELECT person.id, person.age FROM person ORDER BY (person.age + 1) DESC) SQL does not require an enclosing query to preserve the ordering of a derived table, so the rows come back in an arbitrary order. Federated engines that receive this SQL therefore return unordered results for an ordinary `SELECT ... ORDER BY <expression>`. Account for the columns a sort key reads when it is not itself one of the inner `Projection`'s outputs, so such a `Projection` still matches and the `Sort` is hoisted. Hoisting drops the inner `Projection`'s expressions, so the existing dropped-alias substitution now rewrites nested references too: `ORDER BY x + 1` over a dropped `expr AS x` becomes `ORDER BY expr + 1`, not a dangling `x`. Fixes spiceai/spiceai#6931
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.
Summary (root cause)
rewrite_plan_for_sort_on_non_projected_fieldshoists aSortsandwiched between twoProjections to the top of the plan, so the unparsed statement carries the ORDER BY itself. The gate was a set equality on expression strings: the innerProjection's outputs had to be exactly the outerProjection's expressions plus the sort keys.A sort key that is an expression over a projected column never matched that gate.
ORDER BY age + 1contributes the stringperson.age + Int64(1)while the innerProjectionexposesperson.age, so the rewrite bailed out andselect_to_sql_recursivelyfell into itsderived_sortbranch:SQL does not require an enclosing query to preserve the ordering of a derived table. The ORDER BY is buried where the remote engine is free to ignore it, so a federated
SELECT ... ORDER BY <expression>comes back in arbitrary order — silently, with no error.ORDER BY age(a bare column) was unaffected, which is why this went unnoticed: the failure needs the sort key to be computed.Changes
Projection's outputs, account for the columns it reads instead. An innerProjectionthat exists only to expose those columns then matches, and theSortis hoisted as intended. A sort key does not have to be a projected output for the hoist to be valid — it only has to be computable from them.Projection's expressions, so any alias it defined is dropped. The existing dropped-alias substitution only fired when the whole sort expression was that alias; it now substitutes nested references too, soORDER BY x + 1over a droppedexpr AS xbecomesORDER BY expr + 1rather than a danglingx.Test plan
cargo test -p datafusion-sql— 499 passed, 22 failed. Those 22 are pre-existing onspiceai-54: the same 22 tests with byte-identical snapshot diffs fail on an unmodified checkout of the base commit (b8b95926). Verified by diffing the full run output before and after this change — the failure set and every snapshot assertion are unchanged. The delta is497 -> 499passing, i.e. only the two tests added here.cargo test -p datafusion --test tpcds_planning— 198 passed, no change.plan_to_sql) checking that a plan rooted atSortalways emits a top-level ORDER BY: 0 lost before and after, and the same 4 pre-existinggrouping idunparse errors (q27/q36/q70/q86). No plan's output changed.cargo clippy -p datafusion-sql --all-targetsclean;cargo fmtclean for both touched files (expr.rshas pre-existing fmt drift, left alone).Tests added
order_by_over_non_projected_field_stays_top_level— the expression sort key (ORDER BY age + 1), the aggregate-expression variant (ORDER BY max(age) + 1), and the bare-column case that already worked, so the generalisation cannot regress it.order_by_nested_reference_to_dropped_alias— sort key referencing a dropped inner alias from inside a larger expression.Neutered-fix check: with
rewrite.rsreverted and the tests kept, both new tests fail (the emitted SQL is the derived-table form above) while the 33 existingorder_by_*tests still pass. The regression guards genuinely bite.Note on scope
The reported case (spiceai/spiceai#6931, Dremio TPC-DS q91) no longer reproduces through q91 itself on DataFusion 54 — the optimizer now leaves
Sortat the plan root there. The unparser hazard behind it was still live and is what this fixes; the sweep above is the evidence that no TPC-DS query is currently affected while the one-line querySELECT id FROM person ORDER BY age + 1is.