Repository navigation
build: pin datafusion-federation at spiceai/datafusion-federation#90 - #85
Merged
Merged
Conversation
Moves the pin to 95b8d66, which keeps a subquery the analyzer federates on its own from carrying a correlation to a relation it does not scan. Only the pin and its lock entry change; the crate's public API is unchanged.
There was a problem hiding this comment.
🟢 Approved
The pin is valid and consistently updated across both dependency files.
0 open findings
What changed in this PR
Pins datafusion-federation to the upstream correlated-subquery analyzer fix and keeps dependency resolution synchronized.
Changes:
- Updates the dependency revision to
95b8d66. - Updates the matching lockfile source.
| File | Description |
|---|---|
Cargo.toml |
Updates the federation dependency pin. |
Cargo.lock |
Records the same resolved Git revision. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lukekim
added a commit
to spiceai/spiceai
that referenced
this pull request
Oct 7, 2026
…ocal With the SQLite LIKE refusal, TPC-H Q2 on a SQLite accelerator no longer federates whole, and the federation analyzer then federated its correlated scalar subquery on its own. That subquery's outer reference names a table the statement it became never scans, and DataFusion refused the plan with "Correlated scalar subquery must be aggregated to return at most one row". spiceai/datafusion-federation#90 binds such a reference only to a relation the subquery defines itself. datafusion-table-providers pins datafusion-federation from the same git URL, so it moves too (spiceai/datafusion-table-providers#85), which brings its #83 and with it the duckdb-rs pin on spiceai-1.4.4 that the two must share. The ledger rows for both forks are TEMPORARY until those pull requests merge. sqlite_accelerator_answers_a_correlated_subquery_above_a_partly_federated_join is the repo-side guard: it fails at the previous federation pin and passes at this one.
Author
|
This PR changes only the |
lukekim
added a commit
to spiceai/spiceai
that referenced
this pull request
Oct 7, 2026
…iders at 20bf17d spiceai/datafusion-federation#90 now also compares a correlated reference's relation with resolved_eq, so a scan of schema_b.foo does not bind a reference to schema_a.foo. spiceai/datafusion-table-providers#85 follows it. Both ledger rows stay TEMPORARY until those merge.
Author
|
Addressed #85 (review) on its one thread, which is resolved: spiceai/spiceai#14798 pins CI on
|
There was a problem hiding this comment.
🟢 Approved
The manifest and lockfile pins are consistent and aligned with the paired Spice change.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…on-correlated-binding # Conflicts: # Cargo.lock # Cargo.toml
There was a problem hiding this comment.
🟡 Changes recommended
The pin remains inconsistent with the paired Spice revision, risking duplicate crate versions.
1 open finding
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
🟡 Changes recommended
The paired Spice pin remains on a different revision, creating duplicate dependency copies.
2 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
🟡 Changes recommended
The paired Spice PR still pins efa2bb5, which would violate the required revision alignment.
3 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Author
|
Addressed #85 (review) on its threads, now resolved: spiceai/spiceai#14798 at spiceai/datafusion-federation#90 is approved at |
There was a problem hiding this comment.
🔵 Needs a closer look
The pinned upstream PR remains open and requires a specific merge order and merge strategy.
0 open findings
3 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Author
|
Addressed #85 (review) by leaving the merge to a maintainer, as it asks: merge spiceai/datafusion-federation#90 first (approved and clean at |
phillipleblanc
approved these changes
Oct 9, 2026
| datafusion = { version = "55.0", default-features = false } | ||
| datafusion-expr = { version = "55.0" } | ||
| datafusion-federation = { git = "https://github.com/spiceai/datafusion-federation.git", rev = "750561d79e88fd48afd06b6a136e3bc1dc2b8a12" } # branch: spiceai-55 | ||
| datafusion-federation = { git = "https://github.com/spiceai/datafusion-federation.git", rev = "2fc46179fa45b764e8c2996bf04aa58fc652d069" } # branch: spiceai-55 |
lukekim
added a commit
to spiceai/spiceai
that referenced
this pull request
Oct 9, 2026
…iders at ef2e590 spiceai/datafusion-federation#90 and spiceai/datafusion-table-providers#85 were squash-merged into spiceai-55, so the pins move from their branch heads to the squash commits and both TEMPORARY markers come out of the ledger. Each squash commit has the same tree as the head this branch tested (1dc0d36d and 6657c63f), so no code changes.
wangyusheng1985
pushed a commit
to wangyusheng1985/spiceai
that referenced
this pull request
Oct 9, 2026
…NULLs, and unsupported fns (spiceai#14798) * fix(duckdb,sqlite): correct pushdown for string_agg ORDER BY, concat NULLs, and unsupported fns DuckDB's unparser drops aggregate ORDER BY, so a federated string_agg(DISTINCT … ORDER BY …) came back unordered (and memory vs file could disagree). Refuse that shape so DataFusion evaluates it locally; keep unordered string_agg pushable. Render encode(x, 'hex') as lower(hex(x)) so encode(sha256(x), 'hex') is not handed to DuckDB's charset encode. Leave approx_distinct local — DuckDB's approx_count_distinct is a different HyperLogLog. SQLite has no dialect seam, so deny the built-ins it cannot evaluate faithfully: ASCII-only upper/lower, concat that skips NULLs, and the functions it does not have (to_hex, md5, sha256, date_part, date_trunc, regexp_like, regexp_replace, plus ILIKE, median, approx_distinct, and string_agg). Those queries now succeed locally and match the unaccelerated engine. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * fix(sqlite): refuse case-folding LIKE and widen concat coverage SQLite LIKE matches 'alice' against '%ALICE%'; keep it (and ILIKE) local so the accelerator agrees with DataFusion. Expand the SQLite concat fixtures for a typed NULL, all-NULL args, and the empty string, and fix the DuckDB ordered string_agg expected table width. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * test(sqlite): keep a table column in all-NULL concat queries A constant-only SELECT over the SQLite accelerator is an empty federated scan; the row decoder panics on a zero-field schema. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * test(sqlite): pin LIKE emptiness on a distinct projection The results cache returns a schema-less empty batch for the same SQL, which pretty-prints as ++ instead of an empty id table. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * test(sqlite): pin LIKE emptiness by row count A zero-row answer is no batches, which pretty-prints as ++ rather than an empty headered table. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * fix(sqlite): keep encode and remaining regexp builtins local SQLite has no encode, regexp_match, regexp_instr, or regexp_count. A federated call fails with no such function; deny them so they evaluate locally like the other missing builtins. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * test(sqlite): use abs as the shared deny-list control function upper is now a SQLite extra (ASCII-only case folding), so the SQLite/MySQL Spice-function deny-list test probes abs instead. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * fix(duckdb,sqlite): match encode hex case and LIKE test expectations DataFusion accepts only lowercase 'hex', so encode(..., 'HEX') stays local. The older SQLite faithful-cast test now expects LIKE to stay local as well. Co-authored-by: Luke Kim <lukekim@users.noreply.github.com> * fix(duckdb): keep every ordered aggregate local unless its ORDER BY is WITHIN GROUP The unparser renders an aggregate ORDER BY only as WITHIN GROUP and drops it for every other aggregate, so the string_agg-only guard still sent array_agg(x ORDER BY y) to DuckDB as array_agg(x) (answered in DuckDB's order) and first_value/last_value(x ORDER BY y) as functions DuckDB does not have. duckdb_can_translate_aggregate and the opt-in DuckDBAggregateLogicalPushdown marker now refuse any aggregate with a non-empty ORDER BY unless the function supports WITHIN GROUP. Aggregates without an ORDER BY, and WITHIN GROUP aggregates such as percentile_cont, still federate. * fix(duckdb): push ordered aggregates down with their ORDER BY inside the call The unparser renders an aggregate ORDER BY only as WITHIN GROUP, so an ordered string_agg, array_agg, first_value or last_value had to stay local to answer in the right order, which moves every row out of DuckDB. DuckDB takes the ordering inside the call, as DataFusion does. SpiceDuckDBDialect wraps DuckDBDialect, forwarding every Dialect method, and renders those four with the ORDER BY inside the call: first_value and last_value become DuckDB's first and last aggregates, which respect nulls as DataFusion does by default. duckdb_can_translate_aggregate federates exactly those orderings, still keeps any other ordered aggregate local, and now also keeps IGNORE NULLS local, which the unparser never renders. * fix(federation): keep a standalone-federated subquery's correlation local With the SQLite LIKE refusal, TPC-H Q2 on a SQLite accelerator no longer federates whole, and the federation analyzer then federated its correlated scalar subquery on its own. That subquery's outer reference names a table the statement it became never scans, and DataFusion refused the plan with "Correlated scalar subquery must be aggregated to return at most one row". spiceai/datafusion-federation#90 binds such a reference only to a relation the subquery defines itself. datafusion-table-providers pins datafusion-federation from the same git URL, so it moves too (spiceai/datafusion-table-providers#85), which brings its spiceai#83 and with it the duckdb-rs pin on spiceai-1.4.4 that the two must share. The ledger rows for both forks are TEMPORARY until those pull requests merge. sqlite_accelerator_answers_a_correlated_subquery_above_a_partly_federated_join is the repo-side guard: it fails at the previous federation pin and passes at this one. * test(function_support): restore the test imports the decimal revert removed The trunk merge took spiceai#14825's revert of the decimal AVG/SUM refusal, which also dropped the test module's Arc and FunctionSupport imports. The aggregate and window helpers this branch keeps still use both. * fix(sqlite): keep array_agg, first_value, last_value and nth_value aggregates local SQLite has no array_agg, and has first_value, last_value and nth_value only as window functions, so each as an aggregate failed the query with "no such function" or "misuse of window function". The aggregate guard now refuses them. The window guard refuses array_agg only and keeps the three window functions SQLite runs natively. * build: pin datafusion-federation at efa2bb5 and datafusion-table-providers at 20bf17d spiceai/datafusion-federation#90 now also compares a correlated reference's relation with resolved_eq, so a scan of schema_b.foo does not bind a reference to schema_a.foo. spiceai/datafusion-table-providers#85 follows it. Both ledger rows stay TEMPORARY until those merge. * build: pin datafusion-federation at 7a3ad76 and datafusion-table-providers at 4d20ade Both fork pull requests merged their spiceai-55, so the pins now also carry spiceai/datafusion-federation#91: the federation executor asks for the SELECT 1 placeholder a scan with no column unparses to. Without it, a constant-only SELECT over a SQLite acceleration panicked in the row decoder and left the dataset's connection closed, and an aggregate over a constant on DuckDB failed with "Unexpected number of columns. Expected: 0, Found: 1". Both now have guard tests, and the fork-patch ledger has a row for spiceai#91. The federation pin also binds a correlation nested inside a subquery federated on its own to its own query blocks, and matches a qualified correlated reference only against a relation with its qualifiers. * test(duckdb): name the pushdown case tuple, for clippy::type_complexity * fix(sqlite,duckdb): federate only the aggregates and windows SQLite has, and keep IGNORE NULLS windows local The SQLite aggregate guard was a deny-list, so stddev, var_pop, corr, bool_and, bit_and, approx_median and the rest of DataFusion's aggregates still reached SQLite and failed with `no such function`, and `count(a, b)` with `wrong number of arguments`. Aggregates are now an allow-list (count, sum, avg, min and max, each over at most one argument), and a window federates only as one of SQLite's eleven window functions or one of those aggregates. A DISTINCT window and ROLLUP/CUBE/GROUPING SETS, which SQLite refuses, stay local too. The unparser never renders IGNORE NULLS on a window, so lag, lead, first_value, last_value and nth_value with IGNORE NULLS reached SQLite and DuckDB respecting nulls and returned different rows. Both window guards now refuse it, as the DuckDB aggregate guard already did. * build: pin datafusion-federation at 2fc4617 and datafusion-table-providers at ef2e590 spiceai/datafusion-federation#90 and spiceai/datafusion-table-providers#85 were squash-merged into spiceai-55, so the pins move from their branch heads to the squash commits and both TEMPORARY markers come out of the ledger. Each squash commit has the same tree as the head this branch tested (1dc0d36d and 6657c63f), so no code changes. * fix(federation): keep IGNORE NULLS, and an aggregate ORDER BY the answer depends on, local on every source The unparser never renders IGNORE NULLS, and renders an aggregate's argument-list ORDER BY only as WITHIN GROUP. A federated `lag(v) IGNORE NULLS` therefore came back from PostgreSQL respecting nulls, and `array_agg(id ORDER BY id DESC)` in PostgreSQL's own order, with no error. BigQuery and MySQL policies had no guard either. FunctionSupportBuilder::build(), which builds every connector and accelerator policy, now installs aggregate_clauses_survive_unparsing and window_clauses_survive_unparsing, composed with each source's own checks (new aggregate_call / window_call builder methods). An ordering that cannot change the answer (an order-insensitive aggregate, avg, median) or one the source's dialect renders (renders_aggregate_order_by; DuckDB's) still federates. The DuckDB and SQLite guards keep only their engine-specific refusals. The BigQuery federation corpus pins the four statements whose ordered ARRAY_AGG/STRING_AGG now stays local, in the offline test and the release gate script. * docs(udfs-api): say why an ordered avg may drop its ORDER BY The comment called avg order-independent and cited one measurement, but in floating point the order of the additions can change an average, as it can a sum. What makes dropping the clause safe is that DataFusion does not order the additions by it either: over 1e16, -1e16, 1 it answers 0.0 for ORDER BY id, for ORDER BY id DESC and with no ORDER BY at all. * test(bigquery): count the ordered-aggregate partials in the harness self-test The release gate runs `bigquery_corpus.py --self-test` before it sends a query to BigQuery. Its job-count test looked up every EMPTY_BUILD_SKIPPED entry in ROUNDING_CAST_PARTIAL, so the ordered-aggregate entries for 085, 086 and 087 raised KeyError: 85, and the gate would have stopped there. It now reads both pinned tables. A new test pins the harness's ordered-aggregate check: query 226 passes only with its local ordered aggregate and one remote subtree, and a plan with three remote subtrees is right for 085 and wrong for 086 and 226. The fixture README still said the gate requires 269 jobs, one per remote subtree. Since spiceai#14848 it requires the 264 that run; the plans now hold 278. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Luke Kim <lukekim@users.noreply.github.com>
3 tasks done
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.
Moves the
datafusion-federationpin to7a3ad76, the head of spiceai/datafusion-federation#90 after that branch mergedspiceai-55. It carries both that analyzer fix (below) and spiceai/datafusion-federation#91, the empty-projection placeholder fix #86 pinned, so this crate keeps #86's fix.That commit stops the federation analyzer from federating a correlated subquery whole when it is analyzed on its own, which happens once the query around it does not federate whole. The fix is an analyzer change only: no public API of
datafusion-federationchanges, and nothing in this crate needs to.Both pins have to move together. Spice takes
datafusion-federationfrom git at the same URL this crate does, so leaving this pin behind would put two copies of the crate in Spice's graph (seedocs/dev/fork_patches.mdin spiceai/spiceai). Spice bumps both pins in spiceai/spiceai#14798.Merge order: spiceai/datafusion-federation#90 first, with a merge commit, not a squash. This pin names that PR's head commit (
7a3ad76), which stays reachable fromspiceai-55only if that PR merges with a merge commit.Spice-side build of this pair (spiceai/spiceai#14798 working tree, with the fix patched in by path, 2026-10-07):
data_componentsfederation guards: 46 passed.runtimeunit tests: 1673 passed.