Skip to content

feat(sql): lower corr to a two-column extension intent - #420

Open
Selvomega wants to merge 1 commit into
mainfrom
feat/new-corr-agg
Open

Selvomega wants to merge 1 commit into
mainfrom
feat/new-corr-agg

Conversation

@Selvomega

Copy link
Copy Markdown
Collaborator

Why

workload/tpch_deequ.yaml carries two data-quality cells, U-P4d and U-P4l,
that express distributional checks as Pearson correlation. Both failed to lower:

skipping "U-P4d" — lowering failed: unsupported aggregate: corr
skipping "U-P4l" — lowering failed: unsupported aggregate: corr

DataFusion plans corr(x, y) fine; lower_agg_intent rejected it, because
AggSemantic has no correlation. The planner therefore could not see these two
cells at all. Getting corr into the IR is the precondition for any later work
on it.

What

The SQL frontend now lowers corr(x, y) to an AggIntent::Extension, and the
aggregate's output column is typed Float64 / nullable instead of the generic
Utf8 placeholder.

Corpus lowering goes from 39/50 to 41/50. Scope is deliberately limited to
translation plus surviving the optimization pipeline: this PR produces no new
optimization
, and is not intended to.

How

lower_corr mirrors lower_arg_selector and runs before the lookup_native
call. corr differs from argMax in one way — it is a real native DataFusion
aggregate — but the reason it needs handling here is identical: lookup_native
has nothing to return for it.

It lowers to Extension rather than a new core variant. Every core AggIntent
reducer folds one column (col: Option<C>); correlation reads two and would be
the first binary core variant. AggIntent::Extension states its own admission
bar — core only grows for intents that at least two deployment models actually
use — and a repo-wide search found no second model wanting correlation; PromQL
has none. This is the same judgement lower_arg_selector records for
argMax/argMin (issue #232).

Both columns are stored in payload as validated bare-column ColumnRefs,
under x_col / y_col, in written order. Order is load-bearing: a later
rewrite into co-moments has to tell x from y.

The output type is settled in one place. argMax must be patched at both
query_expr.rs call sites because its type follows the selected column;
corr is always Float64 regardless of schema, so output_column resolves it
directly. nullable: true because corr is NULL on zero variance or fewer than
two contributing rows.

Before this PR

U-P4d and U-P4l were rejected at lowering and never reached the planner.

pre.json: 39/50 exported, 11 failed to lower; 173 node(s) over 135 distinct, 1 shared

After this PR

Both cells lower and appear in the exported graph as independent nodes.

pre.json:  41/50 exported, 9 failed to lower; 181 node(s) over 141 distinct, 1 shared
post.json: 41/50 exported, 9 failed to lower; 181 node(s) over 141 distinct, 1 shared; notes SketchApproximationx1

The 9 remaining failures are 8 Date32 and 1 IntervalMonthDayNano — untouched
by this branch and expected; they belong to feat/type-extension.

Developers can now write corr in a workload and have the planner carry it
end to end. It realizes through CostModel::realize_extension, which still
defaults to PassThrough — the seam is left in the right place, not used.

Evidence

Execution example — before/after on the same workload.

$ uv run python pyscripts/asap-planner_test/run_dag_export.py -w workload/tpch_deequ.yaml

before:  skipping "U-P4d" — lowering failed: unsupported aggregate: corr
         skipping "U-P4l" — lowering failed: unsupported aggregate: corr
         39/50 exported, 11 failed to lower

after:   (no corr entry in the skip list)
         41/50 exported, 9 failed to lower

The three corr intents in post.json are distinct and are not folded.
U-P4d and U-P4l use different column pairs, so collapsing them would mean
the payload never entered the hash. It did not:

node 91  (U-P4d): {"ext_kind":"corr","payload":{"x_col":{"Named":"l_quantity"},"y_col":{"Named":"l_extendedprice"}}}
node 117 (U-P4l): {"ext_kind":"corr","payload":{"x_col":{"Named":"l_discount"},"y_col":{"Named":"l_tax"}}}
                  {"ext_kind":"corr","payload":{"x_col":{"Named":"l_quantity"},"y_col":{"Named":"l_discount"}}}

Column pairs and their order match the SQL as written.

The single SketchApproximation note in post.json is attributed to count,
not to corr — consistent with corr taking the default PassThrough path.

Performance measurement: not applicable — no optimization is produced and no
hot path changed.
Screenshot: not applicable — no visual output.
Diagram: not applicable — the design document already carries the
Extension seam discussion.

Verification

  • Unit tests (crates/frontend-sql/tests/sql_lowering.rs, 4 new):

    • corr_lowers_to_an_extension_intentcorr(x, y) produces
      Extension { ext_kind: "corr" } rather than a lowering error.
    • corr_payload_preserves_both_column_names — both columns survive in
      payload, under x_col/y_col, in written order.
    • corr_over_an_expression_binds_the_derived_column — an expression argument
      is carried, not dropped: the planner materializes it into the Project below
      the Aggregate and the payload names that derived column.
    • corr_output_column_is_a_nullable_float — the aggregate's output column is
      Float64 and nullable, not the Utf8 placeholder.

    All 4 pass. Each was written against the design before the implementation
    landed; they were not verified to fail against an unmodified tree as part of
    this run, since this is new behavior rather than a correctness fix.

  • Regression suites: cargo test -p asap-frontend-sql -p asap-types
    321 tests, 0 failures.

  • End-to-end: run_dag_export.py over the full 50-cell corpus, both the
    pre-ASAP and --post-asap runs, no error and no panic. Numbers above.

  • Other checks: cargo fmt --check clean; cargo clippy -p asap-frontend-sql -p asap-types --all-targets clean (exit 0, no warnings).

  • Corpus test / ratchet: deliberately absent. See Limitations.

Architectural decisions

Extension over a new core AggIntent variant. Rejected adding a binary
core variant: correlation would be the first reducer reading two columns, and
Extension's stated bar is two or more deployment models wanting the intent.
PromQL has no correlation, so corr does not clear it today. argMax is the
matching precedent.

Output type resolved in output_column, not patched at the call sites.
Rejected the argMax pattern of patching query_expr.rs at both sites, since
that pattern exists only because the arg selectors' output type follows the
selected column. corr is schema-independent, so one site suffices. Core
already recognizes specific ext_kinds in arg_selector_columns, so this is
not a new kind of coupling.

Columns kept unresolved in payload. Extension has no typed column field,
so core carries the two ColumnRefs without resolving them, per reducer_col's
bare-column rule (issue #115).

Limitations and follow-up

  • No optimization is produced. realize_extension still defaults to
    PassThrough. Overriding it is deferred.
  • The Extension path can only return one candidate. Core cannot enumerate
    alternatives for an opaque shape, so even after realize_extension is
    overridden there will be no "KLL or DDSketch" choice space for corr. This is
    a known property of the seam, not a regression.
  • Cross-query sharing is untested in practice. agg_is_mergeable returns
    true for Extension, so SharedSubtreeStrategy could in principle share two
    identical corr intents. The corpus has no identical pair — all three column
    pairs differ — so the corpus exercises nothing here and 1 shared is unchanged
    from the baseline.
  • No corpus test or ratchet in this PR. tests/data_quality_check/tpch_deequ.rs
    and its .sql are created by feat/type-extension, which has not landed; two
    PRs creating the same files would conflict. corr coverage is backed by the
    unit tests above, and the corpus still records the two cells on the failing
    side.
  • Follow-up, after both branches land: a separate PR raising the DQC lowering
    ratchet from 48 to 50. Note that on dev/dqc the ratchet becomes actually 50
    while pinned at 48 — it must be updated in place on merge or the test goes red.
  • Later extension points, neither needed now: override realize_extension to
    give corr a real implementation form, and add a rewrite strategy matching
    ext_kind == "corr" that decomposes it into six mergeable sums
    (Sx, Sy, Sxy, Sxx, Syy, n), modelled on AvgToSumOverCountStrategy. Matching
    an Extension shape is as feasible as matching a core variant, so this choice
    does not block that path — it only costs one payload decode.

Human review — do not complete with an agent

  • The MVP boundary is correct.
  • New conceptual layers or public interfaces are necessary.
  • The before/after description matches the intended product behavior.
  • Human reviewer:
  • Decision and rationale:

@Selvomega
Selvomega requested a review from zzylol September 15, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant