You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(unparser): derive a Limit that is a join input under the scan's own name, unconditionally (refs spiceai/spiceai#14375) - #234
A Limit that is a join input was given a scope of its own only when the enclosing SELECT already carried a WHERE or a projection (the Limit arm derives when already_projected(), and otherwise asks row_limit_needs_own_scope, which answers has_selection() — a test written for a Filter above a Limit in one scope, not for a join input). Without either, the input's LIMIT landed on the enclosing query, bounding the join's output instead of one input's contribution: with b = {1, 2} and c = {1}, the plan returns two rows and the SQL returned one. And when it was derived, the derived table took the dialect's derived_limit alias — or none on the default dialect — while the join's ON and the select list still named the scan (spiceai/spiceai#14375).
Changes
SelectBuilder counts the join inputs the walk is inside (enter_join_input / leave_join_input / in_join_input); walk_join_input enters and leaves it around each input's walk, whatever the walk returns.
A Limit reached inside a join input is derived through the existing derive_row_limited_scope: under the scan's own name (which the join's ON and select list already use), with the scan's columns as that side's select items, and with that helper's refusals for a qualified name on a dialect that spells columns in full and for an input projecting no columns.
A limit over anything but one scan (a nested join, a grouping) has no name to take: with a projection already above the join it keeps the dialect-alias derived table it had while its output columns are unqualified (a renaming projection); over qualified columns (a grouping, a join), which the derived table would hide from the enclosing ON and select list, or without a projection, it is refused.
A limited scan projecting no columns keeps its SELECT 1 FROM b LIMIT n count scope, as before.
derive_row_limited_scope's refusals name the row limit rather than "a filter applied after a row limit", since a bare limited join input now reaches them.
cargo test -p datafusion-sql --test sql_integration -- limit_join_input_is_derived, with the issue's plan b FULL JOIN (c LIMIT 1) and no projection above.
Before (unparser from origin/spiceai-54, test from this branch):
-SELECT b.id, c.id FROM b FULL JOIN (SELECT c.id FROM c LIMIT 1) AS c ON b.id = c.id
+SELECT b.id, c.id FROM b FULL JOIN c ON b.id = c.id LIMIT 1
test cases::plan_to_sql::limit_join_input_is_derived_under_the_scans_name ... FAILED
After (this branch):
test cases::plan_to_sql::limit_join_input_is_derived_under_the_scans_name ... ok
test cases::plan_to_sql::limit_join_input_projecting_no_columns_keeps_its_count_scope ... ok
Test plan
At head c519f7e70 (merged with spiceai-544cf679211):
cargo test -p datafusion-sql --test sql_integration -- limit_join_input full_join_input_predicate_not_on_a_scan: 5 passed. The first fails on origin/spiceai-54 with the LIMIT on the enclosing query pasted above.
cargo test -p datafusion-sql --test sql_integration -- cases::plan_to_sql: 386 passed, 22 failed, against 382 passed on spiceai-54 alone. The 22 are the same names on both (pre-existing snapshot drift).
Re-stated for head aa545b91 (the fix(unparser): keep a no-op Limit off the join-input derived scope commit, which answers Copilot's no-op-Limit finding). The passes below attest
that commit; the record for the earlier heads is kept underneath it.
Adversarial review: skipped, declared — codex ran against origin/spiceai-54...aa545b912
and exited 1 with its own reason, quoted verbatim: [codex] Codex error: Your workspace is out of credits. Ask your workspace owner to refill in order to continue.grok is not a permitted
engine for a Grok-authored diff, so there is no fallback and no substitute was run.
/security-review: no findings, scoped by hand to aa545b912. The skill's own scoping
resolved to the whole spiceai-54 base delta (2.4 MB of upstream DataFusion) rather than this
commit, so the pass was scoped manually to what this commit writes: one added boolean
conjunct on a branch condition in SQL generation from an already-planned LogicalPlan, plus
one unit test. It adds no input parsing, no I/O, no deserialization, no auth surface, no unsafe, and no dependency; it narrows a branch and never widens one.
/simplify: no change — the commit's production diff is a single added conjunct; there is
no smaller form of it that keeps the two fields distinguishable.
Earlier heads
Adversarial review: skipped — codex exited 1 on this head (81b0ba0), twice: once returning no structured verdict (its partial text repeated the wildcard-sibling shape below), then "Your workspace is out of credits. Ask your workspace owner to refill in order to continue."; grok is not a permitted engine for a Grok-authored diff. The pass on the previous head 42c1f85ab did complete — codex job review-muh7pypf-93sacn, verdict needs-attention, 2 findings: (1) an aliased unprojected scan beside the limited input renders SELECT *, c.id … — refuted by measurement: origin/spiceai-54 renders the same SELECT *, c.id FROM a AS b INNER JOIN c ON b.id = c.id LIMIT 1 for that plan (with the limit misplaced on top), so the duplicated select list is pre-existing and independent of this change; (2) a limited scan projecting no columns under COUNT(*) was refused where the base derived SELECT 1 FROM b LIMIT 3 — confirmed and fixed in 81b0ba0 (the count scope is kept), pinned by limit_join_input_projecting_no_columns_keeps_its_count_scope. The first head bf8a0909a was reviewed too — job review-muh75hom-bnrdir, verdict approve.
/security-review: no findings — the join-input counter is entered and left in walk_join_input around the one recursive call with the result captured first; derive_row_limited_scope unparses the subtree with its own builder, so the shared SELECT's state does not leak into it; no unwrap/expect/indexing added.
/simplify: applied — the enter/walk/leave pattern on both inputs became walk_join_input; the review's point that the unconditional refusal also caught shapes that previously unparsed through the dialect-alias path (a limit over a nested join or projection, with a projection above) was taken: that path is kept when a projection is above, and only the previously-wrong case (no projection, LIMIT on the enclosing query) is refused. Light checks re-run: both tests and clippy --all-targets --no-deps clean on the amended head.
Refs spiceai/spiceai#14375 (closes once spiceai/spiceai moves its datafusion pin past this commit and adds the fork_patches.md row). Touches the same Join arm as #231 and #233; whichever merges later rebases.
…wn name, unconditionally (refs spiceai/spiceai#14375)
A `Limit` that is a join input was given a scope of its own only when the
enclosing SELECT already carried a WHERE or a projection; otherwise its
`LIMIT` landed on the enclosing query, bounding the join's output instead of
one input's contribution to it. And when it was derived, the derived table
took the dialect's `derived_limit` alias — or none — while the join's `ON`
and the select list still named the scan.
The select builder now counts the join inputs the walk is inside; a `Limit`
reached there goes through `derive_row_limited_scope`, which derives it
under the scan's own name with the scan's columns as this side's select
items. A limit over anything but one scan has no name to take and is refused
rather than bounding the wrong thing.
`LogicalPlanBuilder::limit(0, None)` builds a `Limit` with neither `skip`
nor `fetch` — it bounds nothing. Sending it down the join-input path gave
it a derived table it has no rows to justify, so a plan that carried one
unparsed to `INNER JOIN (SELECT c.id FROM c) AS c` instead of `INNER JOIN c`.
Require a bound before taking that branch; a no-op limit falls through to
the path it took before, which recurses without emitting anything.
Measured on this branch, `cargo test -p datafusion-sql`:
- without the guard: 23 failures, including the new
`no_op_limit_join_input_is_passed_through`, whose actual output was
`SELECT b.id, c.id FROM b INNER JOIN (SELECT c.id FROM c) AS c ON b.id = c.id`
- with the guard: 22 failures — the `spiceai-54` base's pre-existing set,
unchanged — and the new test passes
`limit_join_input_is_derived_under_the_scans_name` still passes: its limits
carry `fetch=Some(1)`, so they keep the derived scope this branch gave them.
Fix misleading qualification error for bare limited scans
datafusion/sql/src/unparser/plan.rs:1231
This new call path can reach derive_row_limited_scope for a bare limited scan. On a fully qualified scan with full_qualified_col(), the helper then reports that “a filter applied after a row limit” is unsupported even when the plan has no filter. Generalize the helper's qualification error (or pass the operation context) so join-input failures describe the actual plan.
This issue also appears on line 1242 of the same file.
Both sides added a join-input depth counter on SelectBuilder, bumped around
walking each join input at the same two call sites: spiceai-54 as
`enter_join_input`/`exit_join_input`/`within_join_input`, this branch as
`enter_join_input`/`leave_join_input`/`in_join_input`. The auto-merge kept
both and did not compile (duplicate `enter_join_input`, `walk_join_input`).
Keep spiceai-54's names and have this branch's Limit derivation read
`within_join_input`. The plan_to_sql tests from both sides are kept.
Base merge blocked on a semantic collision with #231, which merged into spiceai-54 a few minutes ago. Not resolving it from the bot side — it needs a decision about the refusal contract, not a merge strategy.
The mechanical part is done and clean.spiceai-54 and this branch both appended tests to the end of plan_to_sql.rs, so the only conflict was append/append; I rebuilt it as both blocks rather than resolving hunk-by-hunk. The merged tree differs from origin/spiceai-54 by exactly this PR's own 150 lines and nothing else:
The semantic part is a genuine opposition.#231 added a case to full_join_input_predicate_not_on_a_scan_is_refused asserting that a filtered scan reached through a row limit, as a FULL JOIN input with no select list taken, must be refused — its stated reason being "nothing lists the scan's columns for a derived table". This PR's derive_join_input_limit makes exactly that shape take the scan's own name in a derived table, which does list those columns. The two cannot both hold.
Reproduced on the merged tree (fcdc48f48):
$ cargo test -p datafusion-sql --test sql_integration full_join_input_predicate_not_on_a_scan_is_refused
test cases::plan_to_sql::full_join_input_predicate_not_on_a_scan_is_refused ... FAILED
panicked at datafusion/sql/tests/cases/plan_to_sql.rs:7710:36:
the scan's columns are unlisted: ...
It no longer refuses; it emits
SELECTa.id, c.idFROM (SELECTa.idFROM a WHERE (a.id='x') LIMIT1) AS a FULL JOIN c ONa.id=c.id
Baselined against the base so the count is not read off a noisy suite — origin/spiceai-54 alone fails 22 of these tests (pre-existing, unrelated: roundtrip_statement*, the snowflake_flatten* family, test_table_scan_pushdown), and the merged tree fails 23. The one added failure is this test, and only this test.
The decision needed: the emitted SQL above looks correct to me — it scopes both the filter and the LIMIT inside the derived table and names the columns the enclosing ON and select list bind to, which is precisely what the FULL JOIN input needs. On that reading this PR supersedes that one case of #231's refusal, and #231's test expectation should change from expect_err to this snapshot. But that is a call about what the refusal contract is for, and it retracts an assertion a maintainer just merged, so I am not making it from the bot side.
Not pushing the resolved merge either. spiceai/datafusion runs no build or test CI, so a push would leave this red test completely invisible on the PR — it would read as green. The resolution is held locally until someone rules on the case above.
Related: #233 is escalated in the same area (the aliased-join early return).
Resolves the append/append conflict at the end of plan_to_sql.rs by keeping
both test blocks.
#231's full_join_input_predicate_not_on_a_scan_is_refused pinned a filtered
scan reached through a LIMIT as a FULL JOIN input as refused, because nothing
listed the scan's columns for a derived table. This branch derives that limit
under the scan's own name with the scan's columns listed, so that case now
unparses to
SELECT a.id, c.id FROM (SELECT a.id FROM a WHERE (a.id = 'x') LIMIT 1) AS a
FULL JOIN c ON a.id = c.id
which keeps the filter and the LIMIT inside the input's scope. The case's
assertion becomes that snapshot; the test's other two cases are unchanged.
…d_scope's refusals
A bare limited scan that is a join input now reaches derive_row_limited_scope,
whose refusals described a filter applied after a row limit even when the plan
has no filter.
Hold lifted: the #231 refusal question in #234 (comment) and #234 (comment) is decided in favour of this PR's behaviour, in d64c103fa (merge of spiceai-54 at 4cf679211).
This PR derives a limited join input under the scan's own name, with its columns listed, so the case that #231's full_join_input_predicate_not_on_a_scan_is_refused pinned as refused now unparses. That test's case now asserts:
SELECTa.id, c.idFROM (SELECTa.idFROM a WHERE (a.id='x') LIMIT1) AS a FULL JOIN c ONa.id=c.id
Checked on DuckDB against the plan's semantics (filter, then take 1 row, then FULL JOIN), with a = {x, x, y} and c = {x, z}: both return (x, x), (NULL, z). The test's other two cases are unchanged. The conflict itself was two test blocks both appended at the end of plan_to_sql.rs, and I kept both. This supersedes the local resolution in #234 (comment).
Also addressed in 728219a5a: the "Previously missed" low finding in #234 (review). All three of derive_row_limited_scope's refusals said "a filter applied after a row limit", which is misleading for a bare limited join input. They now say "a row limit in a scope of its own". Before the fix, the new case in limit_join_input_is_derived_under_the_scans_name (a qualified s.c on a dialect with full_qualified_col) failed with Unparsing a filter applied after a row limit is not supported for a qualified table name…. It passes now.
#234 (review) was a quota error, not a review, so I've re-requested a review.
cargo test -p datafusion-sql --test sql_integration -- cases::plan_to_sql
385 passed; 22 failed (spiceai-54 4cf679211 alone: 382 passed; the same 22 failed, diffed by name)
cargo test -p datafusion-sql --lib 90 passed
cargo clippy -p datafusion-sql --all-targets --all-features --no-deps -- -D warnings clean
The 22 are pre-existing snapshot failures on the base (for example roundtrip_statement expects t1.id and gets id), unrelated to this PR.
…d, unless it is one scan
With a projection above the join, a limit over anything but one scan was
derived without a name its columns could be reached by: a grouping's key or
a join's columns stay qualified by a relation inside the derived table, so
the enclosing ON and select list named an unbound relation, which DuckDB
and PostgreSQL reject. Inputs whose columns are unqualified (a renaming
projection, a scan projecting none) keep the derived table.
Re #234 (review) (🔵, no findings, head c519f7e70): Copilot asks for a human to check the SQL scoping across joins and dialects, so this PR now needs a human approval, not another Copilot round.
…n fixes backported from 54 (#14800)
* build: pin the DataFusion 55 / Arrow 59 fork lines
Moves every fork to its DataFusion 55 line: arrow-rs 59.3.0, DataFusion
55.1.0, Ballista (upstream main on DataFusion 55), federation,
table-providers, iceberg-rust 0.10.1, duckdb-rs 1.4.4 on arrow 59, ADBC 24,
snowflake-rs, spark-connect-rs, spice-rs and spicebench on arrow 59.
datafusion-functions-json comes from crates.io (0.55.6); delta-kernel stays
on 0.27.1 with its default engine split into delta_kernel_default_engine.
docs/dev/fork_patches.md is re-audited against the new lines; the pins stay
marked TEMPORARY until the fork pull requests merge.
* feat: upgrade to DataFusion 55 and Arrow 59
- Statistics flow through DataFusion 55's StatisticsContext. The deprecated
partition_statistics returns Absent for built-in plans on 55, so every
wrapper implements child_stats_requests / statistics_from_inputs and the
optimizer rules, join sizing and Flight batch sizing read real statistics
again.
- ExecutionPlan / TableProvider wrappers implement the new required and
defaulted methods explicitly (apply_expressions, replace_children,
try_to_proto, merge_into, ...), forwarding where they wrap.
- Arrow 59 refuses a MAP whose entries field is nullable: Flight SQL reads
replace the declared schema before decoding and normalize the map
afterwards, and legacy IPC maps decode as a list (arrow_tools map_entries).
- Delta Lake refuses Void and interval columns with a structured error
instead of mis-reading them.
- Ballista 55: Vcores, the protocol-version handshake, task-id based
cancellation and the append-only task-info model.
- Vortex 0.86 and the remaining API moves (TableReference, TableScanBuilder,
TableSchema builder, codec proto converter, WriteOp::MergeInto).
* ci: validate the datafusion-table-providers pin against spiceai-55-patches
The DataFusion 55 line of the fork is spiceai-55-patches.
* revert: drop the unfinished guard tests d65aa29 picked up by accident
d65aa29 was meant to change only the CI branch check; it also committed
work-in-progress test edits. Restore those files to 98800a8; the finished
tests land in their own commit.
* fix(duckdb): push regexp_count down NULL-preserving, as DataFusion 55 evaluates it
DataFusion 55 (apache/datafusion#24239) makes regexp_count propagate NULL,
as PostgreSQL does. The DuckDB rendering still wrapped the count in
coalesce(.., 0) to match the old kernel, so a DuckDB-accelerated dataset
answered 0 where local evaluation answers NULL, and a WHERE on the count kept
rows local evaluation drops. Render it as len(regexp_extract_all(..)), which
is NULL for a NULL input or pattern.
* test(cayenne): count -0.0 as equal to 0.0 in the boundary-values test
DataFusion 55 compares floats in SQL by IEEE 754 value, so float_val = 0.0
matches the -0.0 row too (4 rows, not 3). A MemTable in the same session
returns the same rows, so this is the expectation, not Cayenne.
* build: repin the forks and guard their new patches
- Ballista 8bf37ad0: the executor poll loop accepts a vcore semaphore with no
permits yet. Upstream's assert panicked the loop at startup, so no Spice
executor registered and every cluster query failed.
- DataFusion d98ae645 (MSRV-clean unparser), federation 9ca84a39,
table-providers 661d16d1, iceberg 575d81a1: their own tests now run
against the Spice DataFusion (and, for table-providers, ADBC) forks.
- Guard tests for the unparser rescope, eager aggregation through
StatisticsContext, SchemaCastScanExec statistics forwarding and Ballista
TaskInfo fields 11/12; the ledger names them and records the Ballista fix.
- The distributed EXPLAIN snapshot now reads Ballista 55's
UnknownPartitioning(1) where it printed None.
* build: point the fork pins at the spiceai-*-patches-2 branches
The DataFusion 55 lines of arrow-rs, datafusion, datafusion-ballista,
datafusion-federation, datafusion-table-providers, duckdb-rs, snowflake-rs and
spark-connect-rs are reviewed on new spiceai-*-patches-2 branches, leaving the
earlier spiceai-*-patches branches untouched. Same revisions; only the branch
names in the pin comments, the fork-patch ledger and the table-providers pin
check change.
* fix: re-root a delegated projection on the inner plan before swapping
DataFusion 55's projection pushdown reads a projection's input as the node
being swapped with: `ProjectionExec` collapses the chain starting at
`projection.input()`, `CoalescePartitionsExec` rebuilds from its child. A
wrapper that handed its inner plan the projection unchanged (its input being
the wrapper) got it back still above the wrapper, and the pushdown then nested
another copy on every step: the Cayenne maintained-aggregate soundness test
grew to gigabytes and never finished.
CayenneAccelerationExec, the DuckDB aggregate pushdown marker and
PartitionedUnionExec now rebuild the projection over their inner plan (whose
schema they share) before delegating. Regression tests cover the Cayenne
wrapper and the DuckDB marker.
* fix(cayenne): read legacy inline map data under Arrow 59 and keep the type policy
- Inline data written under a nullable map-entries declaration is read through
arrow_tools::map_entries, since Arrow 59 refuses that declaration at decode.
- The primary-key fast-path test's control query no longer uses a bare SUM,
which DataFusion 55 answers from statistics without a scan.
- RunEndEncoded columns stay refused: the Vortex pin can store them, but
accepting them is a product change, so the drift test records the exception.
* fix: bring the remaining tests and connectors in line with DataFusion 55 and Arrow 59
- Flight: repair a server's nullable map-entries declaration before decoding
(Arrow 59 refuses it), as the Flight SQL path does.
- Federation: SQLite and MySQL refuse a filter on a volatile projection output
(DataFusion fork unparser), Turso forwards it, MSSQL opts out.
- Declared types: parse the decimal form directly so an out-of-range
precision/scale still names the problem under arrow-rs's stricter parser.
- Expectations that DataFusion 55 changed: predicate reordering (reorder_q8),
binary literals rendered as X'..', sorts moved below projections (vector
scans), the NSQL context's version and Spark function list, and the
null-aware anti join corrected in Ballista's distributed planner.
- json_get tests follow datafusion-functions-json 0.55 (negative array
indices, json_as_text for a string cast, integral floats in json_get_int).
* fix(vortex): port the merged point-lookup and key-block code to DataFusion 55 and Vortex 0.86
Trunk's point-lookup and key-block changes were written against DataFusion 54 and
the earlier Vortex pin. The scan now binds its projection and filter before
handing them to Vortex (the reader takes bound expressions), the point read keeps
trunk's optimize-then-bind of its conjuncts, dynamic filters are detected through
DynamicFilterTracking, and TableSchema is built with From. Two merged tests move
to the current Vortex Arrow conversion and DataFusion's non-zero batch_size.
* fix(arrow_tools): keep the producer's dictionary ids when repairing a map declaration
Repairing a nullable map-entries declaration re-encoded the IPC schema, and
re-encoding renumbers dictionaries from zero while the dictionary batches that
follow keep the producer's ids. A producer that numbers them differently then
failed to decode, or decoded each dictionary column against another's dictionary
and returned wrong values with no error (reproduced on the Flight SQL read, the
Flight read and the Flight subscribe paths).
The repair now patches the schema message in place: one byte per map field (the
entries nullability, or the Map-to-List type tag for the decodable form),
verified with the flatbuffers verifier and checked against the expected schema;
an unexpected layout is refused rather than passed through.
* fix(runtime): keep the built-in semantics of functions datafusion-spark 55 would shadow
datafusion-spark 55 registers Spark versions of power/pow, atan2 and concat_ws
over the built-ins (infinity instead of an error for zero to a negative power,
Float64 widening, array flattening) and adds hypot, monthname, quote and weekday.
None of that is a deliberate change, so those functions join trunc, date_trunc and
date_part in the withheld list, and a test pins exactly which names resolve to a
Spark implementation. The NSQL context now lists the functions the session
actually registers, so it no longer advertises the withheld ones.
* fix(duckdb): keep a binary literal out of SQL sent to DuckDB
DataFusion 55's unparser renders a binary literal as X'ff', which DuckDB 1.4.4
reads as the text 'xff': through DuckDB federation, b = X'ff' returned the row
holding the bytes 'xff' instead of 0xFF (and X'' missed the empty blob), with no
error. Expressions with a binary literal now stay local; a NULL still pushes
down. Covered end to end by duckdb_binary_literal_filters_agree_with_local_evaluation.
* build: pin DataFusion at 006f3d21, which drops input sums from join statistics
DataFusion 55 answers a bare SUM from exact statistics, and inner/outer join
statistics carried each input's sum through, so SUM over a join returned the whole
input table's sum (silent wrong results; caught by the Cayenne result-correctness
parity tests). The fork fix is spiceai/datafusion#239; the ledger records it with
those parity tests as its guard.
* test(runtime-datafusion): drop a redundant clone in the binary-literal test
* test: fix the Spark built-ins table width and report the pinned DataFusion revision
- the_built_session_keeps_the_built_in_math_and_string_functions: the expected
table padded one column a character wider than the rendered output; the values
were already right.
- spice-substrait-compliance reports DATAFUSION_FORK_REV, which still named the
pre-upgrade revision; it now matches the workspace pin, as its test requires.
* test(oracle): re-record the pushdown plan for DataFusion 55's decimal literal display
DataFusion 55 prints a decimal literal as Decimal128(123.45,10,2) where 54 printed
Decimal128(Some(12345),10,2). The SQL sent to Oracle is unchanged.
* build: repin the forks to their merged fix PRs and carry spiceai-54 forward
- DataFusion f22d1a74: spiceai-54 forward-merged into the 55 line
(spiceai/datafusion#241), bringing #237 (a Date32 literal's cast uses the
dialect's date type; SQLite read CAST('…' AS DATE) as a number, so date ranges
pushed to SQLite matched nothing) and metadata-column listing pruning.
- arrow-rs be4918e7, Ballista 7c4d54c2, table-providers 3045960a, iceberg 50e982dc,
arrow-adbc 674d6184: the merged CI fix PRs.
- Guard for #237: a Date32 range unparsed for SQLite keeps the rows it selects
(runs the SQL on SQLite with --features sqlite); it fails on the previous pin.
- Ledger rows for both; the substrait harness reports the new DataFusion pin.
* build: pin DataFusion at ae719330, the merge of spiceai/datafusion#241
Same tree as f22d1a74 (spiceai-54 forward-merged into the 55 line); now the head
of spiceai-55-patches-2 rather than a temporary branch.
* style: rustfmt the SQLite date-range federation test
* build: pin arrow-rs at its spiceai-59 merge and DataFusion at spiceai-55-patches-3
- arrow-rs: 2e2cc330, the merge of spiceai/arrow-rs#29 into spiceai-59 (same tree as the previous pin).
- DataFusion: fc84f2d1 on spiceai-55-patches-3, which carries the Date32 literal fix (spiceai/datafusion#237) but not the metadata-column listing pruning (#229); the ledger records that pruning as not carried on 55 until it has a guard here.
- Correct the spark-connect-rs branch annotation to spiceai-59-patches-2.
* build: pin DataFusion at the spiceai-55 merge, and iceberg-rust and snowflake-rs at their arrow-rs bumps
- DataFusion: 02550cf9, the merge of spiceai/datafusion#243 into spiceai-55 (same tree as the previous pin); no longer a temporary pin.
- iceberg-rust: 0284be3d and snowflake-rs: b95a74e8, which move each fork's own arrow-rs pin to 2e2cc330 (Cargo manifests only; Spice builds the same code).
* build: repin ballista, table-providers, iceberg-rust and spark-connect-rs to their branch heads
- datafusion-ballista 557ae4ea: arrow-rs/DataFusion landed pins, and the Rust 1.99 clippy fixes (#70, #71, #72).
- datafusion-table-providers 5f50cfd5 and iceberg-rust a0bd6b04: arrow-rs/DataFusion landed pins (#82, #53).
- spark-connect-rs 3bf6421a: regenerated lockfile.
datafusion-federation and duckdb-rs stay at 9ca84a39 and 8ee43073: table-providers still pins exactly those revisions, and a second revision of either would put two copies of the crate in the graph. Their newer heads only change their own Cargo manifests and lockfile.
* build: pin iceberg-rust, snowflake-rs, spark-connect-rs, spice-rs and spicebench at their merges
The fork PRs these pins pointed at have merged, mostly as squash merges, so
several pinned commits are no longer on their base branches. Each pin now
points at the merge on the fork's base branch:
- iceberg-rust spiceai-0.10.1-df-55 @ 3e2a14e (spiceai/iceberg-rust#50, #54)
- snowflake-rs spiceai-59 @ f555738 (spiceai/snowflake-rs#13, #15)
- spark-connect-rs spiceai-59-2 @ 18ae9bd (spiceai/spark-connect-rs#15, #16)
- spice-rs trunk @ 4429f39 (spiceai/spice-rs#103)
- spicebench trunk @ 8a90555 (spiceai/spicebench#284)
datafusion-federation stays at 9ca84a3, which is on spiceai-55 because
spiceai/datafusion-federation#88 was a merge commit; only its branch note
changes.
What changes for Spice: lint-only rewrites in snowflake-api, and tests in
iceberg-datafusion and spark-connect-core. The other differences are in
the forks' own [patch.crates-io] sections, which cargo ignores for a
dependency. spice-rs's tree is identical.
* build: pin the forks at their landed version-branch revisions
- arrow-adbc 6e4119ac (spiceai-24, #7), iceberg-rust 3e2a14e8 (spiceai-0.10.1-df-55, #50),
datafusion-table-providers 465926a3 (spiceai-55, #80), snowflake-rs f5557381 (spiceai-59, #13),
spark-connect-rs 18ae9bd3 (spiceai-59-2, #15), spice-rs 4429f395 and spicebench 8a90555d (trunk).
- datafusion-federation stays at 9ca84a39, now on spiceai-55 (#88): datafusion-table-providers pins
exactly that revision.
- duckdb-rs stays at 8ee43073 and temporary: #49 was squash-merged, so that revision is not on
spiceai-1.4.4, and datafusion-table-providers pins it, so both have to move together.
- datafusion-ballista is unchanged; its version PR is still open.
* build: pin datafusion-ballista at upstream 55.0.0-rc1
spiceai/datafusion-ballista#73 merged upstream's 55.0.0-rc1 tag into
spiceai-55-patches-2, the head of spiceai/datafusion-ballista#68. #68 is
still open, waiting on license-compliance decisions. Pin 718429f, the #73
merge, until #68 lands on spiceai-55.
ballista-core, -executor, -scheduler, -history and -api-types move from
54.0.0 to 55.0.0; no packages are added or removed in Cargo.lock.
`cargo check -p spiced --locked` passes on rustc 1.98.1.
* docs(fork_patches): record the datafusion-ballista pin at 718429f4 (upstream 55.0.0-rc1, #73)
* build: pin datafusion-ballista at a7c4c585, the merge of spiceai/datafusion-ballista#68 into spiceai-55
* build: pin datafusion-ballista at the spiceai-55 merge of #68spiceai/datafusion-ballista#68 merged into spiceai-55 as a7c4c58, a merge
commit whose tree is identical to 718429f, the previous pin
(`git diff 718429f a7c4c58` is empty). Repoint the pin at the landed
revision and drop the TEMPORARY marker from the fork-patch ledger, as
scripts/check_fork_patches.py asks. The ledger's ballista section now
names spiceai-55 and upstream 55.0.0-rc1 as its base.
Cargo.lock changes only the source revision of the five ballista packages.
* perf(cayenne): keep small Vortex scans whole on the main query path
DataFusion 55 lowered `repartition_file_min_size` from 10 MiB to 1 MiB
(apache/datafusion#22439), so the main Cayenne scan byte-range-splits a 1-10 MiB
dimension table `target_partitions` ways and pays a Vortex footer open per range.
The main scan now opts small scans out below 10 MiB, as the internal and
protected-snapshot scans already do below 256 MiB; Parquet listing scans keep the
new default. `small_snapshot_groups_opt_out_of_repartitioning` now asserts the
main scan's opt-out and fails with the threshold back at 0.
* docs(fork_patches): record duckdb-rs on the protected spiceai-1.4.4-patches-2 head, which table-providers pins
* test(duckdb): build CreateExternalTable with DataFusion 55's locations field
* ci: validate the datafusion-table-providers pin against spiceai-55, where #80 landed
* test(cayenne): size the point-lookup fan-out control above the main scan's 10 MiB opt-out
* fix(data-accelerator-api): implement DataFusion 55's apply_expressions for KeepFirstExec
* test: port trunk's new tests to DataFusion 55 (CreateExternalTable locations, common::TableReference)
* fix(cayenne): keep -0.0 and 0.0 equal in pushed-down float comparisons and memory-mode indexes
DataFusion 55 compares floats with the two zeros equal (apache/datafusion#22835),
while Vortex and the Arrow row encoding order them apart. Spell out both zeros
when a float comparison or IN list with a zero literal is pushed into a Vortex
scan, keep float column-vs-column comparisons above the scan, and hash
memory-mode index keys with the file-mode KeyEncoder.
* build: move to iceberg-rust 0.11.0 (RC) and opendal 0.58
Points the iceberg crates at spiceai/iceberg-rust#55, which merges the
upstream 0.11.x release branch onto the DataFusion 55 fork line.
opendal moves to 0.58 to match iceberg-storage-opendal, so one opendal
stack is built instead of two. 0.58 removes the OperatorBuilder step:
Operator::new returns the operator with the same default layers that
finish() applied.
* Bump datafusion to 55.2-rc1
* build: pin iceberg-rust at spiceai-0.11.0-df-55 (6317d03e)
spiceai/iceberg-rust#55 merged into spiceai-0.11.0-df-55. Same Rust code as
the previous pin (e02547cb); the branch adds only a Python CI pin and a
doc-comment fix. fork_patches.md records the new branch and that SigV4 is
now an AuthManager in the fork's REST catalog.
* build: keep the lockfile's unrelated resolutions from the DataFusion bump
The previous commit's lockfile update also re-resolved five crates with
range requirements (hashbrown, heck, base64). Restore them so the iceberg
repin changes only the iceberg-rust source lines.
* build: pin iceberg-rust at spiceai-0.11.0-df-55 (7735caa2)
Picks up spiceai/iceberg-rust#49 (IcebergTableProvider::catalog and
::table_ident) and #56 (the fork's own DataFusion pin, which does not
affect this build). #49's fork_patches row and its compile guard land
with #14588, which first calls the getters.
* build: pin datafusion at spiceai/datafusion#250 (55.2.0-rc1 and the 54 backports)
Moves the DataFusion fork from 02550cf9 (55.1.0) to 1749ce4e, the head of
spiceai/datafusion#250, which merges into #249: `spiceai-55` at 55.2.0-rc1
(spiceai/datafusion#248) plus the `spiceai-54` patches #233, #234 and #245
carried onto 55, and #250's review fix. The guards for those follow in the
next commit.
The pin names a pull request branch, so the ledger row is marked TEMPORARY
and `scripts/check_fork_patches.py` refuses to let it land until #250 and
#249 merge and the pin moves to the `spiceai-55` merge commit.
The 55.2.0-rc1 merge changes no Spice patch. Outside `Cargo.lock`, the
fork's 02550cf9..27351ab0 diff has the same patch-id as upstream's
55.1.0..55.2.0-rc1 diff, except `datafusion-spark`'s `quote.rs`, which the
fork's backport had already made identical to 55.2.0-rc1's. That backport's
ledger row is dropped; upstream carries the fix as apache/datafusion#25277.
`Cargo.lock` changes only the 36 DataFusion entries.
* test: guard the DataFusion unparser and footer-cache fixes backported from 54
Each backported fix gets a guard here, so the next re-cut of the fork
cannot drop it silently, plus a ledger row naming the guard.
- spiceai/datafusion#233 (#14373): a join that is another join's right
input stays a parenthesised joined table on that join's right.
`a_join_that_is_another_joins_right_input_stays_on_its_right` checks
that `b` is introduced before the outer `ON` names it and that a LEFT
JOIN keeps a filter from inside it out of `WHERE`. With the `sqlite`
feature it runs the SQL on SQLite and compares the rows with
DataFusion's for the same plan.
- spiceai/datafusion#234 (#14375): a `Limit` that is a join input bounds
that input, not the join.
`a_limit_on_a_join_input_bounds_that_input_rather_than_the_join`, with
the same row comparison.
- spiceai/datafusion#250: a projection over a join that passes up a mark
join's mark is refused rather than unparsed with the mark unbound.
`a_mark_a_projection_over_a_join_passes_up_is_refused`.
- spiceai/datafusion#245 (#12952): the file metadata cache frees an
evicted footer's hit counter. 55's `DefaultCache` already does, so 55
carries no patch; `crates/cayenne/tests/footer_cache_hit_counter_test.rs`
pins the behaviour by counting the live heap across 20,000 cold footer
reads into a full cache.
At 02550cf9 the three unparser guards fail. SQLite rejects the nested
join's SQL ("ON clause references tables to its right"), returns 1 row
where the plan returns 2 for `b FULL JOIN (c LIMIT 1)`, and the mark join
renders as `… LEFT OUTER JOIN (b) ON a.id = b.id WHERE NOT c.mark`.
Against a local DataFusion build with the hit-counter prune removed, the
footer-cache guard measures 2,646,176 bytes of heap growth. At 1749ce4e all
four pass, and the footer-cache growth is 0 bytes.
* build: pin datafusion at spiceai/datafusion#249's merge commit
Moves the fork pin from #250's branch head to bebc4d58, the merge of #249
into `spiceai-55`. Its tree is #249's head, cbda233a: 55.2.0-rc1 plus the
backported #233, #234 and #245. The pin names a landed revision, so the
TEMPORARY marker comes off and `scripts/check_fork_patches.py` passes.
#250 is not part of this revision, so its guard and ledger row are removed.
* Update datafusion version to 55.2 in Cargo.toml
* docs(fork_patches): record the unparser frame refactor #249 carries
`cbda233a6` is the fourth Spice commit at the pinned revision. It moves the
join-input bookkeeping that #233 and #234 add out of
`select_to_sql_recursively_inner`, so an unoptimised build's frame stays
where it was. It had no ledger row, so a re-cut could drop it without the
audit noticing.
Its row is a GAP, with an Open gaps entry. What it buys is stack headroom
in a debug build: without it, the fork's `roundtrip_statement` needs
2,112 KiB and overflows a 2 MiB test thread. This repo runs tests on 8 MiB
threads, so no test here sees the difference deterministically. The entry
names the fork-side check to run at a re-cut instead.
* Fix tests
* build: pin DataFusion at spiceai/datafusion#251, so MAX of a partition column skips empty partitions (#14801)
* build: pin datafusion at spiceai/datafusion#251's merge commit
Moves the DataFusion fork pin from #249's merge commit to the head of
`spiceai-55`, which adds two patches on top of it:
- #251: a file that holds no rows gets no partition-column min/max. Without
it, `MIN`/`MAX` of a partition column is answered from the listing's
statistics with the value of a partition whose files are all empty:
`max(p)` answers '99' for a `p=99` directory holding one empty file, where
the rows give '3'.
- #240: the listing prunes files by metadata-column predicates before
opening them and reports those filters `Exact` (#229 on `spiceai-54`).
Neither changes a manifest, so `Cargo.lock` only moves its source revision.
Each patch gets a ledger row and a guard in the listing connector's tests,
which the gate runs as library tests.
* fix(listing): keep every predicate residual on the `_location` fast path
The pin now carries spiceai/datafusion#240, so the inner listing reports a
metadata-column predicate such as `_size < 50` `Exact`.
`MetadataPruningListingTable` handed pushdown classification to the inner
listing whenever a `_location` predicate was present, but its `_location`
fast path opens the named objects and applies no other predicate, so
`_location = x AND _size < 50` returned the rows of a file the `_size`
predicate excludes.
Report every predicate `Inexact` on that path as well, so a residual
`FilterExec` re-applies it. This is #14790's change, which makes the same fix
for partition predicates, taken byte for byte so that whichever of the two
lands second merges cleanly. The new test goes through
`create_listing_table` with `_location` and `_size` enabled.
* test(metadata): expect the listing to answer `_size = 2319` exactly
With spiceai/datafusion#240 the listing prunes files by a metadata-column
predicate and reports it `Exact`, so the plan for `met_size`, which enables
only `_size`, has no residual `Filter`. The scan lists only the four 2319-byte
files; `data_0.parquet` is 2317 bytes, as its S3 `Content-Length` shows.
Regenerated by running `metadata::s3_metadata_columns` and accepted with
`cargo insta accept`. The rest of the test passes unchanged at this pin,
including the `_location` plans and the returned rows.
* fix(data-connector-api): satisfy pedantic clippy in the metadata-pruning listing test
`make lint-rust`'s `--tests` pass rejected two lints in
`listing_table_prunes_files_by_a_metadata_column_predicate`:
`redundant_closure_for_method_calls` (`|group| group.len()`) and
`format_collect` (`map(format!).collect::<String>()`). Use
`FileGroup::len` and build the file contents with `writeln!` into one
`String`. The test is unchanged in what it checks.
---------
Co-authored-by: Luke Kim <[email protected]>
Co-authored-by: Sergei Grebnov <[email protected]>
wangyusheng1985
pushed a commit
to wangyusheng1985/spiceai
that referenced
this pull request
Oct 8, 2026
…n fixes backported from 54 (spiceai#14800)
* build: pin the DataFusion 55 / Arrow 59 fork lines
Moves every fork to its DataFusion 55 line: arrow-rs 59.3.0, DataFusion
55.1.0, Ballista (upstream main on DataFusion 55), federation,
table-providers, iceberg-rust 0.10.1, duckdb-rs 1.4.4 on arrow 59, ADBC 24,
snowflake-rs, spark-connect-rs, spice-rs and spicebench on arrow 59.
datafusion-functions-json comes from crates.io (0.55.6); delta-kernel stays
on 0.27.1 with its default engine split into delta_kernel_default_engine.
docs/dev/fork_patches.md is re-audited against the new lines; the pins stay
marked TEMPORARY until the fork pull requests merge.
* feat: upgrade to DataFusion 55 and Arrow 59
- Statistics flow through DataFusion 55's StatisticsContext. The deprecated
partition_statistics returns Absent for built-in plans on 55, so every
wrapper implements child_stats_requests / statistics_from_inputs and the
optimizer rules, join sizing and Flight batch sizing read real statistics
again.
- ExecutionPlan / TableProvider wrappers implement the new required and
defaulted methods explicitly (apply_expressions, replace_children,
try_to_proto, merge_into, ...), forwarding where they wrap.
- Arrow 59 refuses a MAP whose entries field is nullable: Flight SQL reads
replace the declared schema before decoding and normalize the map
afterwards, and legacy IPC maps decode as a list (arrow_tools map_entries).
- Delta Lake refuses Void and interval columns with a structured error
instead of mis-reading them.
- Ballista 55: Vcores, the protocol-version handshake, task-id based
cancellation and the append-only task-info model.
- Vortex 0.86 and the remaining API moves (TableReference, TableScanBuilder,
TableSchema builder, codec proto converter, WriteOp::MergeInto).
* ci: validate the datafusion-table-providers pin against spiceai-55-patches
The DataFusion 55 line of the fork is spiceai-55-patches.
* revert: drop the unfinished guard tests d65aa29 picked up by accident
d65aa29 was meant to change only the CI branch check; it also committed
work-in-progress test edits. Restore those files to 98800a8; the finished
tests land in their own commit.
* fix(duckdb): push regexp_count down NULL-preserving, as DataFusion 55 evaluates it
DataFusion 55 (apache/datafusion#24239) makes regexp_count propagate NULL,
as PostgreSQL does. The DuckDB rendering still wrapped the count in
coalesce(.., 0) to match the old kernel, so a DuckDB-accelerated dataset
answered 0 where local evaluation answers NULL, and a WHERE on the count kept
rows local evaluation drops. Render it as len(regexp_extract_all(..)), which
is NULL for a NULL input or pattern.
* test(cayenne): count -0.0 as equal to 0.0 in the boundary-values test
DataFusion 55 compares floats in SQL by IEEE 754 value, so float_val = 0.0
matches the -0.0 row too (4 rows, not 3). A MemTable in the same session
returns the same rows, so this is the expectation, not Cayenne.
* build: repin the forks and guard their new patches
- Ballista 8bf37ad0: the executor poll loop accepts a vcore semaphore with no
permits yet. Upstream's assert panicked the loop at startup, so no Spice
executor registered and every cluster query failed.
- DataFusion d98ae645 (MSRV-clean unparser), federation 9ca84a39,
table-providers 661d16d1, iceberg 575d81a1: their own tests now run
against the Spice DataFusion (and, for table-providers, ADBC) forks.
- Guard tests for the unparser rescope, eager aggregation through
StatisticsContext, SchemaCastScanExec statistics forwarding and Ballista
TaskInfo fields 11/12; the ledger names them and records the Ballista fix.
- The distributed EXPLAIN snapshot now reads Ballista 55's
UnknownPartitioning(1) where it printed None.
* build: point the fork pins at the spiceai-*-patches-2 branches
The DataFusion 55 lines of arrow-rs, datafusion, datafusion-ballista,
datafusion-federation, datafusion-table-providers, duckdb-rs, snowflake-rs and
spark-connect-rs are reviewed on new spiceai-*-patches-2 branches, leaving the
earlier spiceai-*-patches branches untouched. Same revisions; only the branch
names in the pin comments, the fork-patch ledger and the table-providers pin
check change.
* fix: re-root a delegated projection on the inner plan before swapping
DataFusion 55's projection pushdown reads a projection's input as the node
being swapped with: `ProjectionExec` collapses the chain starting at
`projection.input()`, `CoalescePartitionsExec` rebuilds from its child. A
wrapper that handed its inner plan the projection unchanged (its input being
the wrapper) got it back still above the wrapper, and the pushdown then nested
another copy on every step: the Cayenne maintained-aggregate soundness test
grew to gigabytes and never finished.
CayenneAccelerationExec, the DuckDB aggregate pushdown marker and
PartitionedUnionExec now rebuild the projection over their inner plan (whose
schema they share) before delegating. Regression tests cover the Cayenne
wrapper and the DuckDB marker.
* fix(cayenne): read legacy inline map data under Arrow 59 and keep the type policy
- Inline data written under a nullable map-entries declaration is read through
arrow_tools::map_entries, since Arrow 59 refuses that declaration at decode.
- The primary-key fast-path test's control query no longer uses a bare SUM,
which DataFusion 55 answers from statistics without a scan.
- RunEndEncoded columns stay refused: the Vortex pin can store them, but
accepting them is a product change, so the drift test records the exception.
* fix: bring the remaining tests and connectors in line with DataFusion 55 and Arrow 59
- Flight: repair a server's nullable map-entries declaration before decoding
(Arrow 59 refuses it), as the Flight SQL path does.
- Federation: SQLite and MySQL refuse a filter on a volatile projection output
(DataFusion fork unparser), Turso forwards it, MSSQL opts out.
- Declared types: parse the decimal form directly so an out-of-range
precision/scale still names the problem under arrow-rs's stricter parser.
- Expectations that DataFusion 55 changed: predicate reordering (reorder_q8),
binary literals rendered as X'..', sorts moved below projections (vector
scans), the NSQL context's version and Spark function list, and the
null-aware anti join corrected in Ballista's distributed planner.
- json_get tests follow datafusion-functions-json 0.55 (negative array
indices, json_as_text for a string cast, integral floats in json_get_int).
* fix(vortex): port the merged point-lookup and key-block code to DataFusion 55 and Vortex 0.86
Trunk's point-lookup and key-block changes were written against DataFusion 54 and
the earlier Vortex pin. The scan now binds its projection and filter before
handing them to Vortex (the reader takes bound expressions), the point read keeps
trunk's optimize-then-bind of its conjuncts, dynamic filters are detected through
DynamicFilterTracking, and TableSchema is built with From. Two merged tests move
to the current Vortex Arrow conversion and DataFusion's non-zero batch_size.
* fix(arrow_tools): keep the producer's dictionary ids when repairing a map declaration
Repairing a nullable map-entries declaration re-encoded the IPC schema, and
re-encoding renumbers dictionaries from zero while the dictionary batches that
follow keep the producer's ids. A producer that numbers them differently then
failed to decode, or decoded each dictionary column against another's dictionary
and returned wrong values with no error (reproduced on the Flight SQL read, the
Flight read and the Flight subscribe paths).
The repair now patches the schema message in place: one byte per map field (the
entries nullability, or the Map-to-List type tag for the decodable form),
verified with the flatbuffers verifier and checked against the expected schema;
an unexpected layout is refused rather than passed through.
* fix(runtime): keep the built-in semantics of functions datafusion-spark 55 would shadow
datafusion-spark 55 registers Spark versions of power/pow, atan2 and concat_ws
over the built-ins (infinity instead of an error for zero to a negative power,
Float64 widening, array flattening) and adds hypot, monthname, quote and weekday.
None of that is a deliberate change, so those functions join trunc, date_trunc and
date_part in the withheld list, and a test pins exactly which names resolve to a
Spark implementation. The NSQL context now lists the functions the session
actually registers, so it no longer advertises the withheld ones.
* fix(duckdb): keep a binary literal out of SQL sent to DuckDB
DataFusion 55's unparser renders a binary literal as X'ff', which DuckDB 1.4.4
reads as the text 'xff': through DuckDB federation, b = X'ff' returned the row
holding the bytes 'xff' instead of 0xFF (and X'' missed the empty blob), with no
error. Expressions with a binary literal now stay local; a NULL still pushes
down. Covered end to end by duckdb_binary_literal_filters_agree_with_local_evaluation.
* build: pin DataFusion at 006f3d21, which drops input sums from join statistics
DataFusion 55 answers a bare SUM from exact statistics, and inner/outer join
statistics carried each input's sum through, so SUM over a join returned the whole
input table's sum (silent wrong results; caught by the Cayenne result-correctness
parity tests). The fork fix is spiceai/datafusion#239; the ledger records it with
those parity tests as its guard.
* test(runtime-datafusion): drop a redundant clone in the binary-literal test
* test: fix the Spark built-ins table width and report the pinned DataFusion revision
- the_built_session_keeps_the_built_in_math_and_string_functions: the expected
table padded one column a character wider than the rendered output; the values
were already right.
- spice-substrait-compliance reports DATAFUSION_FORK_REV, which still named the
pre-upgrade revision; it now matches the workspace pin, as its test requires.
* test(oracle): re-record the pushdown plan for DataFusion 55's decimal literal display
DataFusion 55 prints a decimal literal as Decimal128(123.45,10,2) where 54 printed
Decimal128(Some(12345),10,2). The SQL sent to Oracle is unchanged.
* build: repin the forks to their merged fix PRs and carry spiceai-54 forward
- DataFusion f22d1a74: spiceai-54 forward-merged into the 55 line
(spiceai/datafusion#241), bringing spiceai#237 (a Date32 literal's cast uses the
dialect's date type; SQLite read CAST('…' AS DATE) as a number, so date ranges
pushed to SQLite matched nothing) and metadata-column listing pruning.
- arrow-rs be4918e7, Ballista 7c4d54c2, table-providers 3045960a, iceberg 50e982dc,
arrow-adbc 674d6184: the merged CI fix PRs.
- Guard for spiceai#237: a Date32 range unparsed for SQLite keeps the rows it selects
(runs the SQL on SQLite with --features sqlite); it fails on the previous pin.
- Ledger rows for both; the substrait harness reports the new DataFusion pin.
* build: pin DataFusion at ae719330, the merge of spiceai/datafusion#241
Same tree as f22d1a74 (spiceai-54 forward-merged into the 55 line); now the head
of spiceai-55-patches-2 rather than a temporary branch.
* style: rustfmt the SQLite date-range federation test
* build: pin arrow-rs at its spiceai-59 merge and DataFusion at spiceai-55-patches-3
- arrow-rs: 2e2cc330, the merge of spiceai/arrow-rs#29 into spiceai-59 (same tree as the previous pin).
- DataFusion: fc84f2d1 on spiceai-55-patches-3, which carries the Date32 literal fix (spiceai/datafusion#237) but not the metadata-column listing pruning (spiceai#229); the ledger records that pruning as not carried on 55 until it has a guard here.
- Correct the spark-connect-rs branch annotation to spiceai-59-patches-2.
* build: pin DataFusion at the spiceai-55 merge, and iceberg-rust and snowflake-rs at their arrow-rs bumps
- DataFusion: 02550cf9, the merge of spiceai/datafusion#243 into spiceai-55 (same tree as the previous pin); no longer a temporary pin.
- iceberg-rust: 0284be3d and snowflake-rs: b95a74e8, which move each fork's own arrow-rs pin to 2e2cc330 (Cargo manifests only; Spice builds the same code).
* build: repin ballista, table-providers, iceberg-rust and spark-connect-rs to their branch heads
- datafusion-ballista 557ae4ea: arrow-rs/DataFusion landed pins, and the Rust 1.99 clippy fixes (spiceai#70, spiceai#71, spiceai#72).
- datafusion-table-providers 5f50cfd5 and iceberg-rust a0bd6b04: arrow-rs/DataFusion landed pins (spiceai#82, spiceai#53).
- spark-connect-rs 3bf6421a: regenerated lockfile.
datafusion-federation and duckdb-rs stay at 9ca84a39 and 8ee43073: table-providers still pins exactly those revisions, and a second revision of either would put two copies of the crate in the graph. Their newer heads only change their own Cargo manifests and lockfile.
* build: pin iceberg-rust, snowflake-rs, spark-connect-rs, spice-rs and spicebench at their merges
The fork PRs these pins pointed at have merged, mostly as squash merges, so
several pinned commits are no longer on their base branches. Each pin now
points at the merge on the fork's base branch:
- iceberg-rust spiceai-0.10.1-df-55 @ 3e2a14e (spiceai/iceberg-rust#50, spiceai#54)
- snowflake-rs spiceai-59 @ f555738 (spiceai/snowflake-rs#13, spiceai#15)
- spark-connect-rs spiceai-59-2 @ 18ae9bd (spiceai/spark-connect-rs#15, spiceai#16)
- spice-rs trunk @ 4429f39 (spiceai/spice-rs#103)
- spicebench trunk @ 8a90555 (spiceai/spicebench#284)
datafusion-federation stays at 9ca84a3, which is on spiceai-55 because
spiceai/datafusion-federation#88 was a merge commit; only its branch note
changes.
What changes for Spice: lint-only rewrites in snowflake-api, and tests in
iceberg-datafusion and spark-connect-core. The other differences are in
the forks' own [patch.crates-io] sections, which cargo ignores for a
dependency. spice-rs's tree is identical.
* build: pin the forks at their landed version-branch revisions
- arrow-adbc 6e4119ac (spiceai-24, spiceai#7), iceberg-rust 3e2a14e8 (spiceai-0.10.1-df-55, spiceai#50),
datafusion-table-providers 465926a3 (spiceai-55, spiceai#80), snowflake-rs f5557381 (spiceai-59, spiceai#13),
spark-connect-rs 18ae9bd3 (spiceai-59-2, spiceai#15), spice-rs 4429f395 and spicebench 8a90555d (trunk).
- datafusion-federation stays at 9ca84a39, now on spiceai-55 (spiceai#88): datafusion-table-providers pins
exactly that revision.
- duckdb-rs stays at 8ee43073 and temporary: spiceai#49 was squash-merged, so that revision is not on
spiceai-1.4.4, and datafusion-table-providers pins it, so both have to move together.
- datafusion-ballista is unchanged; its version PR is still open.
* build: pin datafusion-ballista at upstream 55.0.0-rc1
spiceai/datafusion-ballista#73 merged upstream's 55.0.0-rc1 tag into
spiceai-55-patches-2, the head of spiceai/datafusion-ballista#68. spiceai#68 is
still open, waiting on license-compliance decisions. Pin 718429f, the spiceai#73
merge, until spiceai#68 lands on spiceai-55.
ballista-core, -executor, -scheduler, -history and -api-types move from
54.0.0 to 55.0.0; no packages are added or removed in Cargo.lock.
`cargo check -p spiced --locked` passes on rustc 1.98.1.
* docs(fork_patches): record the datafusion-ballista pin at 718429f4 (upstream 55.0.0-rc1, spiceai#73)
* build: pin datafusion-ballista at a7c4c585, the merge of spiceai/datafusion-ballista#68 into spiceai-55
* build: pin datafusion-ballista at the spiceai-55 merge of spiceai#68spiceai/datafusion-ballista#68 merged into spiceai-55 as a7c4c58, a merge
commit whose tree is identical to 718429f, the previous pin
(`git diff 718429f a7c4c58` is empty). Repoint the pin at the landed
revision and drop the TEMPORARY marker from the fork-patch ledger, as
scripts/check_fork_patches.py asks. The ledger's ballista section now
names spiceai-55 and upstream 55.0.0-rc1 as its base.
Cargo.lock changes only the source revision of the five ballista packages.
* perf(cayenne): keep small Vortex scans whole on the main query path
DataFusion 55 lowered `repartition_file_min_size` from 10 MiB to 1 MiB
(apache/datafusion#22439), so the main Cayenne scan byte-range-splits a 1-10 MiB
dimension table `target_partitions` ways and pays a Vortex footer open per range.
The main scan now opts small scans out below 10 MiB, as the internal and
protected-snapshot scans already do below 256 MiB; Parquet listing scans keep the
new default. `small_snapshot_groups_opt_out_of_repartitioning` now asserts the
main scan's opt-out and fails with the threshold back at 0.
* docs(fork_patches): record duckdb-rs on the protected spiceai-1.4.4-patches-2 head, which table-providers pins
* test(duckdb): build CreateExternalTable with DataFusion 55's locations field
* ci: validate the datafusion-table-providers pin against spiceai-55, where spiceai#80 landed
* test(cayenne): size the point-lookup fan-out control above the main scan's 10 MiB opt-out
* fix(data-accelerator-api): implement DataFusion 55's apply_expressions for KeepFirstExec
* test: port trunk's new tests to DataFusion 55 (CreateExternalTable locations, common::TableReference)
* fix(cayenne): keep -0.0 and 0.0 equal in pushed-down float comparisons and memory-mode indexes
DataFusion 55 compares floats with the two zeros equal (apache/datafusion#22835),
while Vortex and the Arrow row encoding order them apart. Spell out both zeros
when a float comparison or IN list with a zero literal is pushed into a Vortex
scan, keep float column-vs-column comparisons above the scan, and hash
memory-mode index keys with the file-mode KeyEncoder.
* build: move to iceberg-rust 0.11.0 (RC) and opendal 0.58
Points the iceberg crates at spiceai/iceberg-rust#55, which merges the
upstream 0.11.x release branch onto the DataFusion 55 fork line.
opendal moves to 0.58 to match iceberg-storage-opendal, so one opendal
stack is built instead of two. 0.58 removes the OperatorBuilder step:
Operator::new returns the operator with the same default layers that
finish() applied.
* Bump datafusion to 55.2-rc1
* build: pin iceberg-rust at spiceai-0.11.0-df-55 (6317d03e)
spiceai/iceberg-rust#55 merged into spiceai-0.11.0-df-55. Same Rust code as
the previous pin (e02547cb); the branch adds only a Python CI pin and a
doc-comment fix. fork_patches.md records the new branch and that SigV4 is
now an AuthManager in the fork's REST catalog.
* build: keep the lockfile's unrelated resolutions from the DataFusion bump
The previous commit's lockfile update also re-resolved five crates with
range requirements (hashbrown, heck, base64). Restore them so the iceberg
repin changes only the iceberg-rust source lines.
* build: pin iceberg-rust at spiceai-0.11.0-df-55 (7735caa2)
Picks up spiceai/iceberg-rust#49 (IcebergTableProvider::catalog and
::table_ident) and spiceai#56 (the fork's own DataFusion pin, which does not
affect this build). spiceai#49's fork_patches row and its compile guard land
with spiceai#14588, which first calls the getters.
* build: pin datafusion at spiceai/datafusion#250 (55.2.0-rc1 and the 54 backports)
Moves the DataFusion fork from 02550cf9 (55.1.0) to 1749ce4e, the head of
spiceai/datafusion#250, which merges into spiceai#249: `spiceai-55` at 55.2.0-rc1
(spiceai/datafusion#248) plus the `spiceai-54` patches spiceai#233, spiceai#234 and spiceai#245
carried onto 55, and spiceai#250's review fix. The guards for those follow in the
next commit.
The pin names a pull request branch, so the ledger row is marked TEMPORARY
and `scripts/check_fork_patches.py` refuses to let it land until spiceai#250 and
spiceai#249 merge and the pin moves to the `spiceai-55` merge commit.
The 55.2.0-rc1 merge changes no Spice patch. Outside `Cargo.lock`, the
fork's 02550cf9..27351ab0 diff has the same patch-id as upstream's
55.1.0..55.2.0-rc1 diff, except `datafusion-spark`'s `quote.rs`, which the
fork's backport had already made identical to 55.2.0-rc1's. That backport's
ledger row is dropped; upstream carries the fix as apache/datafusion#25277.
`Cargo.lock` changes only the 36 DataFusion entries.
* test: guard the DataFusion unparser and footer-cache fixes backported from 54
Each backported fix gets a guard here, so the next re-cut of the fork
cannot drop it silently, plus a ledger row naming the guard.
- spiceai/datafusion#233 (spiceai#14373): a join that is another join's right
input stays a parenthesised joined table on that join's right.
`a_join_that_is_another_joins_right_input_stays_on_its_right` checks
that `b` is introduced before the outer `ON` names it and that a LEFT
JOIN keeps a filter from inside it out of `WHERE`. With the `sqlite`
feature it runs the SQL on SQLite and compares the rows with
DataFusion's for the same plan.
- spiceai/datafusion#234 (spiceai#14375): a `Limit` that is a join input bounds
that input, not the join.
`a_limit_on_a_join_input_bounds_that_input_rather_than_the_join`, with
the same row comparison.
- spiceai/datafusion#250: a projection over a join that passes up a mark
join's mark is refused rather than unparsed with the mark unbound.
`a_mark_a_projection_over_a_join_passes_up_is_refused`.
- spiceai/datafusion#245 (spiceai#12952): the file metadata cache frees an
evicted footer's hit counter. 55's `DefaultCache` already does, so 55
carries no patch; `crates/cayenne/tests/footer_cache_hit_counter_test.rs`
pins the behaviour by counting the live heap across 20,000 cold footer
reads into a full cache.
At 02550cf9 the three unparser guards fail. SQLite rejects the nested
join's SQL ("ON clause references tables to its right"), returns 1 row
where the plan returns 2 for `b FULL JOIN (c LIMIT 1)`, and the mark join
renders as `… LEFT OUTER JOIN (b) ON a.id = b.id WHERE NOT c.mark`.
Against a local DataFusion build with the hit-counter prune removed, the
footer-cache guard measures 2,646,176 bytes of heap growth. At 1749ce4e all
four pass, and the footer-cache growth is 0 bytes.
* build: pin datafusion at spiceai/datafusion#249's merge commit
Moves the fork pin from spiceai#250's branch head to bebc4d58, the merge of spiceai#249
into `spiceai-55`. Its tree is spiceai#249's head, cbda233a: 55.2.0-rc1 plus the
backported spiceai#233, spiceai#234 and spiceai#245. The pin names a landed revision, so the
TEMPORARY marker comes off and `scripts/check_fork_patches.py` passes.
spiceai#250 is not part of this revision, so its guard and ledger row are removed.
* Update datafusion version to 55.2 in Cargo.toml
* docs(fork_patches): record the unparser frame refactor spiceai#249 carries
`cbda233a6` is the fourth Spice commit at the pinned revision. It moves the
join-input bookkeeping that spiceai#233 and spiceai#234 add out of
`select_to_sql_recursively_inner`, so an unoptimised build's frame stays
where it was. It had no ledger row, so a re-cut could drop it without the
audit noticing.
Its row is a GAP, with an Open gaps entry. What it buys is stack headroom
in a debug build: without it, the fork's `roundtrip_statement` needs
2,112 KiB and overflows a 2 MiB test thread. This repo runs tests on 8 MiB
threads, so no test here sees the difference deterministically. The entry
names the fork-side check to run at a re-cut instead.
* Fix tests
* build: pin DataFusion at spiceai/datafusion#251, so MAX of a partition column skips empty partitions (spiceai#14801)
* build: pin datafusion at spiceai/datafusion#251's merge commit
Moves the DataFusion fork pin from spiceai#249's merge commit to the head of
`spiceai-55`, which adds two patches on top of it:
- spiceai#251: a file that holds no rows gets no partition-column min/max. Without
it, `MIN`/`MAX` of a partition column is answered from the listing's
statistics with the value of a partition whose files are all empty:
`max(p)` answers '99' for a `p=99` directory holding one empty file, where
the rows give '3'.
- spiceai#240: the listing prunes files by metadata-column predicates before
opening them and reports those filters `Exact` (spiceai#229 on `spiceai-54`).
Neither changes a manifest, so `Cargo.lock` only moves its source revision.
Each patch gets a ledger row and a guard in the listing connector's tests,
which the gate runs as library tests.
* fix(listing): keep every predicate residual on the `_location` fast path
The pin now carries spiceai/datafusion#240, so the inner listing reports a
metadata-column predicate such as `_size < 50` `Exact`.
`MetadataPruningListingTable` handed pushdown classification to the inner
listing whenever a `_location` predicate was present, but its `_location`
fast path opens the named objects and applies no other predicate, so
`_location = x AND _size < 50` returned the rows of a file the `_size`
predicate excludes.
Report every predicate `Inexact` on that path as well, so a residual
`FilterExec` re-applies it. This is spiceai#14790's change, which makes the same fix
for partition predicates, taken byte for byte so that whichever of the two
lands second merges cleanly. The new test goes through
`create_listing_table` with `_location` and `_size` enabled.
* test(metadata): expect the listing to answer `_size = 2319` exactly
With spiceai/datafusion#240 the listing prunes files by a metadata-column
predicate and reports it `Exact`, so the plan for `met_size`, which enables
only `_size`, has no residual `Filter`. The scan lists only the four 2319-byte
files; `data_0.parquet` is 2317 bytes, as its S3 `Content-Length` shows.
Regenerated by running `metadata::s3_metadata_columns` and accepted with
`cargo insta accept`. The rest of the test passes unchanged at this pin,
including the `_location` plans and the returned rows.
* fix(data-connector-api): satisfy pedantic clippy in the metadata-pruning listing test
`make lint-rust`'s `--tests` pass rejected two lints in
`listing_table_prunes_files_by_a_metadata_column_predicate`:
`redundant_closure_for_method_calls` (`|group| group.len()`) and
`format_collect` (`map(format!).collect::<String>()`). Use
`FileGroup::len` and build the file contents with `writeln!` into one
`String`. The test is unchanged in what it checks.
---------
Co-authored-by: Luke Kim <[email protected]>
Co-authored-by: Sergei Grebnov <[email protected]>
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
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
A
Limitthat is a join input was given a scope of its own only when the enclosingSELECTalready carried aWHEREor a projection (theLimitarm derives whenalready_projected(), and otherwise asksrow_limit_needs_own_scope, which answershas_selection()— a test written for aFilterabove aLimitin one scope, not for a join input). Without either, the input'sLIMITlanded on the enclosing query, bounding the join's output instead of one input's contribution: withb = {1, 2}andc = {1}, the plan returns two rows and the SQL returned one. And when it was derived, the derived table took the dialect'sderived_limitalias — or none on the default dialect — while the join'sONand the select list still named the scan (spiceai/spiceai#14375).Changes
SelectBuildercounts the join inputs the walk is inside (enter_join_input/leave_join_input/in_join_input);walk_join_inputenters and leaves it around each input's walk, whatever the walk returns.Limitreached inside a join input is derived through the existingderive_row_limited_scope: under the scan's own name (which the join'sONand select list already use), with the scan's columns as that side's select items, and with that helper's refusals for a qualified name on a dialect that spells columns in full and for an input projecting no columns.ONand select list, or without a projection, it is refused.SELECT 1 FROM b LIMIT ncount scope, as before.derive_row_limited_scope's refusals name the row limit rather than "a filter applied after a row limit", since a bare limited join input now reaches them.limit_join_input_is_derived_under_the_scans_name(bareFULL/INNER, projectedFULL, limit on the left input, qualified scan refused on a full-path dialect)limit_join_input_projecting_no_columns_keeps_its_count_scopeandlimit_join_input_over_qualified_outputs_is_refused. fix(unparser): keep a FULL JOIN input's scan filters scoped when the input is a join (refs spiceai/spiceai#12593) #231's FULL JOINLIMITrefusal case now asserts the derived SQL.Reproduction
cargo test -p datafusion-sql --test sql_integration -- limit_join_input_is_derived, with the issue's planb FULL JOIN (c LIMIT 1)and no projection above.Before (unparser from
origin/spiceai-54, test from this branch):After (this branch):
Test plan
At head
c519f7e70(merged withspiceai-544cf679211):cargo test -p datafusion-sql --test sql_integration -- limit_join_input full_join_input_predicate_not_on_a_scan: 5 passed. The first fails onorigin/spiceai-54with theLIMITon the enclosing query pasted above.cargo test -p datafusion-sql --test sql_integration -- cases::plan_to_sql: 386 passed, 22 failed, against 382 passed onspiceai-54alone. The 22 are the same names on both (pre-existing snapshot drift).cargo test -p datafusion-sql --lib: 90 passed.cargo clippy -p datafusion-sql --all-targets --all-features --no-deps -- -D warnings: clean.full_join_input_predicate_not_on_a_scan_is_refusedcase for a filtered scan under aLIMITnow asserts the derived SQL instead of a refusal; that SQL returns the plan's rows on DuckDB.Review gate
Re-stated for head
aa545b91(thefix(unparser): keep a no-op Limit off the join-input derived scopecommit, which answers Copilot's no-op-Limitfinding). The passes below attestthat commit; the record for the earlier heads is kept underneath it.
codexran againstorigin/spiceai-54...aa545b912and exited 1 with its own reason, quoted verbatim:
[codex] Codex error: Your workspace is out of credits. Ask your workspace owner to refill in order to continue.grokis not a permittedengine for a Grok-authored diff, so there is no fallback and no substitute was run.
/security-review: no findings, scoped by hand toaa545b912. The skill's own scopingresolved to the whole
spiceai-54base delta (2.4 MB of upstream DataFusion) rather than thiscommit, so the pass was scoped manually to what this commit writes: one added boolean
conjunct on a branch condition in SQL generation from an already-planned
LogicalPlan, plusone unit test. It adds no input parsing, no I/O, no deserialization, no auth surface, no
unsafe, and no dependency; it narrows a branch and never widens one./simplify: no change — the commit's production diff is a single added conjunct; there isno smaller form of it that keeps the two fields distinguishable.
Earlier heads
codexexited 1 on this head (81b0ba0), twice: once returning no structured verdict (its partial text repeated the wildcard-sibling shape below), then "Your workspace is out of credits. Ask your workspace owner to refill in order to continue.";grokis not a permitted engine for a Grok-authored diff. The pass on the previous head 42c1f85ab did complete —codexjobreview-muh7pypf-93sacn, verdict needs-attention, 2 findings: (1) an aliased unprojected scan beside the limited input rendersSELECT *, c.id …— refuted by measurement:origin/spiceai-54renders the sameSELECT *, c.id FROM a AS b INNER JOIN c ON b.id = c.id LIMIT 1for that plan (with the limit misplaced on top), so the duplicated select list is pre-existing and independent of this change; (2) a limited scan projecting no columns underCOUNT(*)was refused where the base derivedSELECT 1 FROM b LIMIT 3— confirmed and fixed in 81b0ba0 (the count scope is kept), pinned bylimit_join_input_projecting_no_columns_keeps_its_count_scope. The first head bf8a0909a was reviewed too — jobreview-muh75hom-bnrdir, verdict approve./security-review: no findings — the join-input counter is entered and left inwalk_join_inputaround the one recursive call with the result captured first;derive_row_limited_scopeunparses the subtree with its own builder, so the sharedSELECT's state does not leak into it; nounwrap/expect/indexing added./simplify: applied — the enter/walk/leave pattern on both inputs becamewalk_join_input; the review's point that the unconditional refusal also caught shapes that previously unparsed through the dialect-alias path (a limit over a nested join or projection, with a projection above) was taken: that path is kept when a projection is above, and only the previously-wrong case (no projection,LIMITon the enclosing query) is refused. Light checks re-run: both tests andclippy --all-targets --no-depsclean on the amended head.Refs spiceai/spiceai#14375 (closes once
spiceai/spiceaimoves itsdatafusionpin past this commit and adds thefork_patches.mdrow). Touches the sameJoinarm as #231 and #233; whichever merges later rebases.