Skip to content

feat: ASAPPlanner now relies on DataFusion 54.1 - #614

Merged
Selvomega merged 1 commit into
mainfrom
chore/issue-611-datafusion-54
Oct 5, 2026
Merged

feat: ASAPPlanner now relies on DataFusion 54.1#614
Selvomega merged 1 commit into
mainfrom
chore/issue-611-datafusion-54

Conversation

@Selvomega

@Selvomega Selvomega commented Oct 4, 2026 •
edited by zzylol
Loading

Copy link
Copy Markdown
Collaborator

Closes #611.

Why

asap-frontend-sql, the only crate that uses DataFusion, was on 43. Newer versions plan more SQL, and the longer we wait, the bigger the upgrade.

What

DataFusion 43 → 54.1 (arrow 53 → 58, sqlparser 0.51 → 0.62).

Most of the diff is mechanical API migration. DataFusion 54 also changed five behaviors we depend on:

DataFusion 54 change Handling
SELECT * is expanded at planning time It now lowers to an ordinary identity Project. SQL top-k recognition (MaintainedPopulationStrategy) expected Limit { Sort { Scan } }, so it now skips an identity projection under Sort
Errors are wrapped in Collection and Diagnostic SqlError unwraps them, so callers still get DataFusion(Plan(..))
GenericDialect supports aggregate FILTER Our GenericWithAggregateFilter wrapper (dialect.rs) is removed
map(...) is reserved by DataFusion; map() is rejected ClickHouse map is planned as asap_map_construct and renamed back to map during lowering
date - date returns Int64 days instead of a Duration Still rejected, as before

Before / after

Two bgp_jan2024_workload queries now lower:

Query Before After
q128: … / greatest(countIf(…), 1) Invalid function 'greatest' Lowered
q129: IN (SELECT … ORDER BY count(*) DESC LIMIT 5) UnsupportedFeature Lowered

SQL top-k with an explicit, in-order column list (SELECT latency, job … ORDER BY latency DESC LIMIT 5) is now recognized. Before, only SELECT * was.

Tests

  • bgp tally: Lowered 154 → 156, Plan 40 → 39, UnsupportedFeature 6 → 5.
  • Three sql_lowering tests no longer expect SELECT * to elide the Project.
  • Multi-column IN (subquery) is rejected by DataFusion first, so the test's expected message changed.
  • quantile_cont, a new alias of percentile_cont, is added to KNOWN_UNMAPPED_NATIVE_FUNCTIONS.
  • New: top-k over an identity column list is recognized; over a reordered list it is not.

cargo test --workspace: 1570 passed. Clippy is clean.

@Selvomega
Selvomega requested a review from zzylol October 4, 2026 22:34
zzylol
zzylol reviewed Oct 5, 2026
View reviewed changes
Comment thread crates/frontend-sql/src/sql/dialect.rs Resolved
@Selvomega
Selvomega merged commit d4869a7 into main Oct 5, 2026
3 checks passed
zzylol added a commit that referenced this pull request Oct 7, 2026
@zzylol @claude
fix: integrate with #614 (DataFusion 54) in the unified SQL lowering
25c1fdc 
Port DF 54's API changes (Expr::Literal metadata, Cast/TryCast field,
ScalarUDFImpl return_field_from_args/invoke_with_args, catalog path,
dialect name) and the planner behaviour changes (SELECT * keeps its
projection, GenericDialect parses aggregate FILTER) from main's
crates/frontend-sql/src/sql onto the unified copy and its tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 7, 2026
@zzylol @claude
fix: integrate with #614 (DataFusion 54): let chrono resolve to DF 54…
fc65b26 
…'s version

DataFusion 54 requires chrono ^0.4.44, so the runtime crate's exact
=0.4.39 pin no longer resolves; keep 0.4.39 as the minimum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 12, 2026
@zzylol @claude
fix: integrate with #614 (DataFusion 54): let chrono resolve to DF 54…
f92cb39 
…'s version

DataFusion 54 requires chrono ^0.4.44, so the runtime crate's exact
=0.4.39 pin no longer resolves; keep 0.4.39 as the minimum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request Oct 12, 2026
Merged
3 tasks
zzylol added a commit that referenced this pull request Oct 12, 2026
@zzylol @claude
feat(sql): lower SQL to unified operator and scalar IR (#539)
b800a2d 
* feat: lower SQL through unified operators and scalars

* fix: integrate with #614 (DataFusion 54) in the unified SQL lowering

Port DF 54's API changes (Expr::Literal metadata, Cast/TryCast field,
ScalarUDFImpl return_field_from_args/invoke_with_args, catalog path,
dialect name) and the planner behaviour changes (SELECT * keeps its
projection, GenericDialect parses aggregate FILTER) from main's
crates/frontend-sql/src/sql onto the unified copy and its tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* style: apply cargo fmt

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(sql): fold numeric casts of literals as DataFusion does

Type coercion turns a literal compared with a column of another type into
a cast: `latency < 100` on a DOUBLE column became
`latency < CAST(100 AS DOUBLE)`, which summary coverage cannot read as a
constant. Fold a cast of a literal to an integer or float type with
DataFusion's `ScalarValue::cast_to`, the evaluation its constant folding
uses; the predicate lowers as `latency < 100.0`. A cast that fails to
evaluate is kept.

DataFusion's whole SimplifyExpressions rule is not run: it also folds
timestamp casts into timestamp literals, which the IR cannot write yet, and
rewrites REGEXP_LIKE into the `~` operator (#650).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* fix(sql): address #539 review findings

- Column resolution: a qualified reference falls back to the bare name
  only when exactly one column has it, so `ORDER BY b.k` after a join no
  longer binds `a.k`; an ambiguous name is an error. A bare name must be
  unique too.
- Reducer arguments keep their qualifier, so `SUM(b.x)` over a join
  reads `b.x`, not `a.x`.
- `WITHIN GROUP (ORDER BY x DESC)` percentiles use `1 - q`, as DataFusion.
- `CAST(NULL AS ...)` is not folded, so a typed NULL keeps its type.
- Non-literal `LIMIT`/`OFFSET` expressions are rejected instead of dropped.
- A cast in a reducer argument that can change values or the result type
  (`SUM(CAST(i AS DOUBLE))`) is computed as a derived column; only
  widening casts that leave the result unchanged read the bare column.
- Window `FILTER`, `DISTINCT` and `IGNORE NULLS` are rejected.
- `LIKE ... ESCAPE` is rejected.
- DataFusion 54: an integer cast to `Decimal128(_, 0)` (how DataFusion
  compares unsigned with signed integers, e.g. `ROW_NUMBER() ... <= 5`) is
  dropped after type coercion; any other decimal cast, such as a user's
  `CAST(1.5 AS DECIMAL(10, 0))`, is rejected. Type coercion runs after the
  duration check, so a temporal subtraction inside a CASE branch is still
  reported as an unsupported duration.
- A reducer argument is read from the bare column only when the cast is
  directly over a column: coercion wraps `avg(CAST(f AS BIGINT))` as
  `CAST(CAST(f AS Int64) AS Float64)`, and the inner cast truncates.
- A CAST / TRY_CAST of a non-literal to INT, SMALLINT, TINYINT or REAL is
  rejected: the canonical types are 64-bit, so its range check or 32-bit
  rounding would be lost.
- `CAST(x AS VARCHAR)` (DataFusion 54's `Utf8View`) lowers to Utf8.
- A `Concat` column is nullable if any child's is, so a ROLLUP / CUBE /
  GROUPING SETS key is nullable in the combined output.

The last five were found by running 573 queries through DataFusion and
through this lowering.

Each has a regression test in unified_sql_lowering.rs that fails without
the fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 12, 2026
@zzylol @claude
fix: integrate with #614 (DataFusion 54): let chrono resolve to DF 54…
955912b 
…'s version

DataFusion 54 requires chrono ^0.4.44, so the runtime crate's exact
=0.4.39 pin no longer resolves; keep 0.4.39 as the minimum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 12, 2026
@zzylol @claude
fix: integrate with #614 (DataFusion 54): let chrono resolve to DF 54…
d9befb8 
…'s version

DataFusion 54 requires chrono ^0.4.44, so the runtime crate's exact
=0.4.39 pin no longer resolves; keep 0.4.39 as the minimum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 12, 2026
@zzylol @claude
feat(runtime): export timed plans as PhysicalASAPDAG and compile them (…
b4539f7 
…#541)

* feat(ir): restore execution timing assignment for physical plans

The logical export is phase-free. Physical planning still needs the lifecycle
timing expansion; bring it back unchanged except for carrying coverage.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(ir): export timed plans as PhysicalASAPDAG

The logical export carries no execution timing. Physical planning output
needs it: reuse the logical payloads and node ids, and add each node's and
edge's data state plus window compatibility, as the earlier post-ASAP export
did. Coverage is carried through.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(runtime): compile unified operator and scalar graphs

* feat(ir): give physical ASAP DAGs one root per batch query

Match the logical export: a batch is one DAG whose roots are its queries,
with shared sub-DAGs exported once. The runtime compiles all roots.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: integrate with #614 (DataFusion 54): let chrono resolve to DF 54's version

DataFusion 54 requires chrono ^0.4.44, so the runtime crate's exact
=0.4.39 pin no longer resolves; keep 0.4.39 as the minimum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(runtime): read Operator<NodeId> payloads from the physical ASAP DAG

Follow #537: the physical DAG now carries each node's operator as
Operator<NodeId> (from ir::flat) instead of the removed wire mirror types.
The runtime matches Operator::NonASAP / Operator::ASAP directly, and
restore() rebuilds the in-memory DAG with map_children instead of a
field-by-field conversion. Node ids are plain usize.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(ir): follow derived summary coverage in timing and physical export

Coverage is derived from the node (#646), so timed copies no longer carry
it and PhysicalASAPDAGNode drops its coverage field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* fix(runtime): address #541 review findings

- timing: a SummaryAgg the lifecycle assignment recomputes at query time
  now forces the SummaryAgg consuming it to query time
  (`forces_query_time` reads the assignment), instead of failing with
  EvaluationUnderMaintenance.
- physical export: `PhysicalASAPDAG::validate` requires each node's
  incoming edges to be exactly its operator's inputs with their roles, so
  swapped Left/Right edges cannot reorder `a - b` (EdgeRoleMismatch).
- physical export: a DAG that fails validation (an empty batch, a
  hand-timed plan) is an `InvalidPhysicalDAG` error instead of a panic.
- precompute: only a TimeRange over (a TimeShift over) a time series Scan
  takes the raw-sample input contract, as before the port; a table range
  needs a stored population.
- compile: an input binding below a node the PromQL fallback evaluates
  is an error; the fallback reads raw series slots, so it was silently
  ignored. A binding below another bound input is still superseded by
  it, as in `cut`.
- compile: drop the scalar-literal binary branch; nothing filled its map.
- PromQL fallback: clamp's `NOT (max < min)` is now the IR `Filter`
  #540 emits; the fallback runs a filter on scalars alone, and the
  pointwise projection no longer filters on its own.
- tests: `raw_series_contract_is_explicit` asserts that a consumed bare
  selector is instant selection (its comment said the opposite) and that
  a binding below the fallback is rejected; new `unified_precompute` test.
- PromQL fallback: unary minus drops the metric name like every other
  pointwise projection, following #540 and the Prometheus 3.15 baseline
  (#511); pointwise_projection_names_and_dynamic_parameters expects it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* refactor(runtime): remove the PromQL fallback evaluator

ASAPPlanner generates candidates and computes their cost, or says why a
query has none. Executing what the native operators cannot run belongs
to the deployment runtime (ASAPQuery backend), not to the Planner.

Remove unified_physical_planner::promql_fallback and its tests, and the
fallback paths in compile: raw-series input slots, the fallback lowering
and the below-fallback binding check. A node without a native lowering
now fails compilation with Error::Invalid.

Reject Planner node IDs above u32::MAX, which could collide with helper
IDs, and error instead of debug-asserting when a node needs more than
2^16 helpers. Document that forces_query_time also forces an unaligned
per-series PromQL BinaryOp to query time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol deleted the chore/issue-611-datafusion-54 branch November 3, 2026 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zzylol zzylol zzylol approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Why not upgrade the DataFusion version we are using from 43 to 54?

2 participants

@Selvomega @zzylol