Code quality - #46
Closed
bobjansen wants to merge 42 commits into
Closed
Code quality#46bobjansen wants to merge 42 commits into
bobjansen wants to merge 42 commits into
Conversation
bobjansen
force-pushed
the
code-quality
branch
from
September 17, 2026 21:14
3ac7f41 to
ffd3c79
Compare
bobjansen
force-pushed
the
code-quality
branch
from
September 17, 2026 21:17
ffd3c79 to
a538fa0
Compare
SQL's rule: with no `by` the whole input is one group, even when it is empty. The result is now always exactly one row: count(), count(col) and count_distinct(col) are 0, every other aggregate is null. With `by`, an empty input still has no groups and no rows. The kernels are unchanged and still return zero rows; the row is added once at the node level (global_aggregate_of_empty), by interpret_node for the materialized path and by a wrapper operator for the streaming one. count(col) is lowered as a Sum over a not-null flag, so AggSpec gains `is_count` to tell it apart from a real sum; lowering sets it and codegen carries it through make_agg. SPEC 7.1 states the rule. Two tests that pinned the old zero-row answer now expect the row; a parity case covers codegen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A correlated subquery is decorrelated into a grouped aggregate left-joined onto the outer rows, so an outer key with no inner rows got a null. That is right for every aggregate except a count, whose value over no rows is 0: `0 == scalar(... count() ...)` dropped rows SQL keeps. A bare count(), count(col) or count_distinct(col) is now compared through coalesce(value, 0). A count inside a larger expression (`count() + 1`) is rejected, since its value over no rows would have to be evaluated rather than assumed. The uncorrelated form needed no change of its own: its cross join now always has the one row an empty aggregate without `by` yields. SPEC 5.7 no longer claims the old behaviour was SQL's and documents the rule. Regression tests cover correlated count(), count(col) and count_distinct, uncorrelated count() and count(col), and the rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parser rejected `map { }` anywhere but last in a block, and lowering
then required it to be the only clause (SPEC C25). A block like
`t[filter x > 0, map { .. }]` passed the first rule and failed the
second. Clause combinations are lowering's concern, alongside
select/update exclusivity, so the parser check is gone and the one
remaining error states the actual rule.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
min and max now accept String and Categorical columns, grouped and
ungrouped, in both the materialized and the streaming aggregate. Strings
compare byte-wise (UTF-8 code point order, as Polars orders them); a
Categorical compares by its text, never its codes. The sorted operator
already hands non-numeric input to the hash operator, and non-numeric
kinds already stay off the partial-merge paths, so both run serially
like string first/last.
Three existing bugs surfaced while testing nulls:
- string first/last on a group with only nulls hit an internal
invariant (append_scalar on an empty slot) in both implementations;
such a cell is now written as a placeholder under its null bit;
- the materialized first/last took the first/last row even when null,
where SPEC 3.5 says first/last non-null value;
- the materialized first/last were always reported valid, so
`update { f = first(x) }, by g` broadcast 0 to an all-null group.
The type-gate error no longer names HashAggregateState.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Table { x = [1, null, 3] }` previously failed with "series literal
elements must be literals": `null` lowers to a `__null` call, and both
series-literal builders accepted only LiteralExpr. A null element now
clears that row's validity bit instead of storing a value.
`null` carries no type, so the column takes the type of its first
non-null element and a null is never a mixed-type error. An all-null
column has no type to take and falls back to Int64, matching `col = []`.
A standalone `let s = [1, null]` is still rejected, with a reason: a
Series binding is a bare column with no validity bitmap to record it.
ConstructColumn gains a `valid` vector parallel to `elements`. Each null
slot holds a placeholder of the column's own type, so the three
consumers read `elements` uniformly and consult `valid` only to build
the bitmap. That invariant is what makes reading the column type off
element 0 safe for a null-leading column; a default-constructed
placeholder would retype `[null, 1.5]` to Int64, so it is stated at the
read site and pinned by tests that put the null first.
Schema inference claimed Nullability::Never for every literal column,
reasoning that the surface language had no null literal to write. That
is a proof later passes may fold `is null` against, so it is now
conditioned on `valid`. The identical comment in nullability.cpp and the
is-null folding in canonicalize.cpp concern literals in expression
position, which a written `null` never lowers to; both stay as they are.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`a == outer(x) && b == outer(x)` was rejected as a "duplicate capture", though it is a meaningful query: inner rows where both columns equal that outer row's `x`. So was `a == outer(x) && a == outer(y)`, which matches only where the two outer values agree. Neither was a judgement about the query. Decorrelation grouped the subquery by its inner keys and renamed each inner key to its outer name, so the join could match on one shared name -- and two columns cannot both be named `x`. The guard existed to stop that collision. JoinKey already carries `left` and `right` separately, with `fold_output` to keep a differently-spelled pair as one output column; the rename predates it. Carrying the pair per key expresses the query directly and drops the Rename node from every correlated plan: one outer column can be the `left` of several keys. Group keys are deduplicated, since one inner column captured twice is still one group key, and an exactly repeated capture is now redundant rather than an error. The two lowering tests that pinned the old shape assert the new one; the plan diagram and the parity case header that described the rename are updated. A new parity case covers both forms over data where a plan that dropped the second capture would answer differently, not merely stop erroring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two captures sharing an outer column give the decorrelated join two folded keys with the same `left`. Tracing that shape through the passes that read `fold_output`: - join_reorder only reorders Inner joins, so the Left join decorrelation builds is never reordered; - plan_join_output absorbs both right columns and emits one `k`, but records a folded peer on the single left column and so keeps only the first. The peer is read only for a row missing its own side, which Left and Inner never have on the left -- so this is sound here and would not be for Right or Outer; - nullability consults the peer only for those same kinds, and column_origins declines to assign an origin to a folded key on any non-Inner join; - join_pushdown would build a rename list producing one name twice, which ColumnNameMap::validate rejects. It is not reachable: the residual filter above the join always references the generated scalar column, which is right-only, so the destination is always Above. mapped_join_keys explicitly declines to create a repeated fold target, which leaves decorrelation its only producer. The constraint that it must keep emitting Left is recorded where the keys are built, and the output shape is pinned by a join_output test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A consumer builds its result from the chunks an operator hands it, so an
operator that filters every row away and then yields no chunk at all
reports a table with no COLUMNS rather than no ROWS. Downstream that is
not an empty answer but a missing schema:
let a = Table { k = [1, 2], v = [10, 20] };
let b = Table { k = [7, 8] };
(a semi join b on k)[select { n = count(), s = sum(v) }];
error: aggregate: column 'v' not found in input
for a column the input plainly has. A filter that keeps nothing has
always reported its columns, and an inner join over the same empty input
answers 0 -- only the membership joins lost the schema.
ChunkedSemiAntiJoinOperator now keeps a row-less copy of the left's
layout, taken from the first chunk it sees, and emits it on the way out
when it has emitted nothing else. Both paths need it: the streaming one
and the swapped one, which buffers the left when the right is too large
to set-ify. A left that yields no chunk at all leaves nothing to take a
layout from, so that case is unchanged.
The tests cover no-key-matches, an already-empty left, an anti join that
removes everything, and the swapped path above its 65536-row threshold;
each fails without the fix, and a surviving-row case holds the normal
path down. Interleaved A/B at SF-8 on eight cores: q21 1.41s -> 1.32s
median (drift; the added work is one predictable branch per chunk), q04
0.25s unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit gave ChunkedSemiAntiJoinOperator a hand-written schema fallback. `SchemaCarrier` already existed in map_chunked.cpp, where five operators use it for exactly this, and its comment describes the same failure the fix was chasing: "an operator that emits no chunk emits no schema either ... fails with 'unknown column' on what is really just an empty input". The membership joins had simply never adopted it. The copy was also worse. `SchemaCarrier` keeps the held chunk's `ChunkIdentity`; the hand-written one dropped it, so the fallback chunk went downstream with sequence and row offset zeroed. The streaming path now carries the input chunk's identity. The swapped path cannot: it buffers Tables, and the conversion drops identity before the operator sees it, so the default stands and says so. `SchemaCarrier`, `ChunkIdentity`, `chunk_identity_of` and the identity- preserving `table_to_chunk` move from map_chunked.cpp's anonymous namespace to chunk_conversion_internal.hpp, which owns the chunk/table boundary and which both callers already include. `hold` gains `holding()` for a caller whose empty Table costs something to build: `filter_chunk` here reports "nothing survived" as nullopt rather than as an empty table, so there is no zero-row result to hand over and one has to be built from the input's columns -- once, not per chunk. No behaviour change beyond the identity, and 9 fewer lines. Full suite and parity green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A decorrelated subquery grouped the WHOLE inner relation while the left join above it read only the groups the outer rows keyed into. On a q17-shaped query -- a selective outer filter over a large inner table -- that is most of the work. The aggregate's input is now semi-joined against the outer's keys. A semi join keeps every inner row whose key appears in the outer, so each surviving group keeps ALL of its rows and its aggregate is unchanged; the groups it drops could never have been matched. The keys come from the outer as it entered the FIRST decorrelation join, not from the join's output. Every such join is a LEFT join, which neither adds nor removes an outer row, so the captured column holds the same values either way -- but cloning the output would re-run the previous subquery's aggregate to find them, and again for each subquery after it. Taking them from below costs one evaluation of the outer however many subqueries the filter holds. TPC-H q17 written in correlated form, SF-8 on eight cores, interleaved: 1.60s -> 0.60s median, same answer. The same query decorrelated by hand is 0.12s; the rest is the inner relation being scanned again for the subquery and once more for the keys, which sharing a scan would fix and this does not. Not gated on cost. The shape that loses is the inverted one -- a large outer and a small inner -- where cloning the outer buys little. Ranking that needs the source statistics the optimizer is given and the lowerer is not, so a gate belongs with the rewrite moved into a pass, where it would also catch the general case: any left join over an aggregate grouped by the join key, however it was written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The key restriction collects the outer's keys by evaluating the outer a
SECOND time. That is only sound if the second evaluation produces the
same rows, and nothing checked it. An outer that draws from an RNG gives
one set of keys to the join and a different set to the restriction, so
the semi join keeps the groups for the second draw and the rows built
from the first match nothing:
let o = Table { j = [1, 2, 3] }[update { k = Int64(floor(rand_uniform(1.0, 10.99))) }];
o[filter 0 < scalar(wide[filter a == outer(k), select { m = min(v) }])]
Every drawn key exists in `wide`, so all three rows must survive.
Transpiled, this answered 0: `rand_uniform` was emitted twice and drawn
twice. The interpreter got 3 only because it materializes a `let`, which
made this an engine divergence as well as a wrong answer, and no parity
case had a nondeterministic outer to catch it.
`ir::is_replayable_subplan` decides it, as an allow-list: a node kind
nobody has classified is unsafe rather than assumed pure, and so is a
call whose callee is not a known built-in, since an extern is left
unclassified on purpose. `FnKind::Generator` is refused outright.
This costs the optimization on TPC-H q17, 0.60s back to 1.49s. At
lowering a reader is still an ExternCall -- `hoist_extern_sources` turns
it into a Scan later, in the batch planner -- so the guard cannot tell a
parquet read from a plugin and refuses it. That is the honest answer
here and the reason to run this as a pass over the planned tree instead,
where the scans and the source statistics both exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The key restriction moves out of decorrelation and into `restrict_aggregates_to_probed_keys`, run over the planned tree. It had to move to work at all: at lowering a reader is still an ExternCall, so the replayability guard could not tell a parquet read from a plugin call and refused every real query -- q17 went back to 1.49s. After `hoist_extern_sources` it is a Scan, and the source statistics exist too. Matching the plan rather than the syntax generalises it. Any Left or Inner join whose right side is an aggregate grouped by the join key qualifies, however the query was written, and a hand-written one does: TPC-H q20 is 0.41s -> 0.33s. The correlated q17 is back to 0.58s from 1.60s. Nothing else in the 22 moved -- q19 looked 12% worse on two samples and was 0.24s either way on six. The gate counts GROUPS, not rows. The saving is groups the aggregate never builds, so the question is whether the probe side keys into fewer distinct values than the aggregate would group. Costing it in rows says the opposite on the query it was written for: q17 reads the same lineitem table on both sides, so by rows neither is smaller, while by distinct keys the probe wants 25k of 400k groups. `distinct_estimate` stops at a join and the probe side usually is one, so the descent is local to this pass rather than a change to a function join ordering costs itself against. `is_replayable_subplan` becomes `clone_replayable_subplan`: the copy and the safety test are now one switch, so a kind that cannot be copied is exactly a kind that must not be replayed and the two cannot drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Table(n)` holds its size in `row_count` and has no columns at all, but
`clone_node` rebuilt a Construct from its COLUMNS. Whole-program lowering
inlines a `let` by cloning, so the clone was an empty frame:
let scaffold = Table(3)[update { k = 7 }];
scaffold;
printed three rows under `ibex_eval` and none transpiled. Written
without the `let` it was always correct, which is why the existing
`construct_deferred_row_count` case never showed it.
The guard is a lowering test, not a parity case. `structured_runner`
builds its reference by calling `parser::lower` as well, so the two
sides share one lowered plan and a bug in lowering moves both together:
a parity case written for this passed before the fix and after it. What
diverged was `ibex_eval`, which plans through the pipeline in repl.cpp,
against the transpiler. The case added here is kept for the emitter
coverage it does give, and says plainly what it does not.
Found while auditing an unrelated wrong answer, where this was the real
cause of a transpiled query returning 0 rows and sent me looking in the
wrong place for a while.
Separately and not fixed: a `let` over `Table(n)` that is USED twice
becomes a shared source and aborts with "ScanNode cannot be emitted".
That one is loud, is not a wrong answer, and predates this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`lower_program` asks `lower_script` to share a `let` referenced from
several table positions: the plan comes back separately in
`shared_bindings` and every reference is left as a `Scan(name)` for the
batch executor to resolve through its registry. It then handled `sinks`
(a clear error) and `preamble` (spliced in) -- and dropped the shared
bindings on the floor. The scans dangled, and the emitter aborted:
let scaffold = Table(3)[update { k = 7 }];
(scaffold join scaffold on k)[select { n = count() }];
ibex_compile: ScanNode cannot be emitted
`lower()` has to return one self-contained tree, so the plans are now
spliced back in at their references, in declaration order, across the
preamble, the result and later bindings' plans.
A shared binding has two or more references by construction, so all but
one are clones -- and a clone is a second evaluation. Clones go through
`clone_replayable_subplan`, so a binding drawing from `rand_uniform()`
is a lowering error naming the binding rather than a compiled program
quietly disagreeing with `ibex_eval`, which materializes it once.
`Construct` was missing from the replayable allow-list, which refused
even `Table(3)[update { k = 7 }]`; both its forms are handled now. The
walk for the largest node id existed in three copies, so it becomes one
`ir::max_node_id` beside the clone whose id counter needs it.
The two `test_lower.cpp` cases were run against the unfixed tree first:
without the splice the plan keeps two dangling scans and no Construct,
and the `rand_uniform()` case returns a plan where an error belongs.
1946 tests green, parity 44/44 exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bobjansen
force-pushed
the
code-quality
branch
from
September 21, 2026 08:46
7daa883 to
bd909c3
Compare
- Share is_reorderable_inner_join between costing and rebuild so join reordering declines take/nulls/expect/suffix clauses anywhere in a chain - Skip filter and semi/anti pushdown through joins with take first/last/any or cardinality assertions (would pick a different match or hide a violation) - Non-equi join predicates now carry validity for both sides so a null's payload never compares as a real value - Add regression tests and parity cases Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The restriction inserts a `nulls never` semi join under the aggregate, which would drop the null-keyed group a `nulls equal` join must still match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- `take first/last/any` collapses matches per left row, which on right and outer joins silently dropped a right row whose matches all went to other left rows. It is now emitted null-padded, as an unmatched right row is. SPEC.md states this. - The deferred-probe pass declines `nulls equal` joins; its dynamic filter over the build keys does not model null-to-null matches. The two shape tests in scan_predicates now share one predicate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit said the dynamic filter mishandles null keys under `nulls equal`. Tracing the runtime shows it never gets the chance: the build-key filter is published only by the streaming inner-join operators, and `is_streamable_inner_join` / `is_streamable_pair_int_join` already exclude `nulls equal`, `take` and `expect`. Such a join takes the materialized path, where the deferred scan's filter is never marked ready and the scan decodes unfiltered. With the guard removed the answer is still correct (verified on a real parquet repro and the e2e test below), so the guard is defense in depth, not a bug fix. - Reword the comment to say that, and make the plan-time shape test mirror all three runtime exclusions instead of only `nulls equal`. - Add an e2e lazy test running a `nulls equal` and a `nulls never` join over a deferrable probe with null keys on both sides; the lazy test source now carries column validity through its decode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A UINT64 column keeps its footer min/max in unsigned order, while the fused
key scan and the static-range filter compare the same bits as signed int64.
A row group spanning 2^63 read back as min > max, so `group_max < lo` held
and the whole group was skipped: `t[filter k >= 5 && k <= 7, select {
n = count() }]` returned 0 on a file whose k column held 0..99998 plus
2^63+3, where the int64 twin returns 3.
Pruning and the whole-file span now trust a footer range only when it is
ordered as signed int64 (libs/parquet/stats_range.hpp, unit-tested without
Arrow). The planner's merge_column_stats gets the same guard so it does not
derive a span from an inverted range.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`integer_literal` accepted a Date literal without knowing the column, so `t[filter k > date"2020-01-01" && k < date"2030-01-01"]` on an Int column was answered from raw int64 bits (0 rows) by the reader's fast path, while the ordinary filter raises "cannot compare date and non-date". Move `static_range_filter` into its own TU taking the source schema, and accept a Date literal only against a Date column and an Int literal only against an Int or Date column (the ordinary filter compares a date with a day count too). Anything else, including Double and String columns, is left to the ordinary path up front instead of being declined later by the reader. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The fused key scan answered a filter only when it was entirely literal comparisons on one integer column that the query did not also output. Two common shapes fell to decoding whole predicate columns: a time range whose key is also selected, and a range plus any other conjunct. And `ts >= X`, having no upper bound, pruned no row groups at all. - `split_static_range` absorbs the comparisons on the first answerable integer column and hands back the remaining conjuncts; the source narrows the fused selection with them (as join_key_selection already does) instead of declining the whole predicate. When the range leaves no rows, the remainder is still evaluated over the zero-row schema table so a type error in it is reported as on the ordinary path -- declining there instead sent every empty streamed unit back to the slow path. - Row-group pruning needs only one bound. - The reader declines up front when row groups wholly inside the interval hold more than `footer_pass_rate_limit` of the rows (0.75; 0.5 when the key is also output and gets decoded twice), from footer statistics alone. Without it wide ranges regressed: 36 -> 54 ms at 90% pass with the key in the output. - A dynamic membership filter keeps the old exact shape, so the filter this path would drop is still applied. Release, 10M rows, 50 row groups, sorted key, 8 cores, interleaved A/B, min of 7 (ms, before -> after): 1% range with key output 32 -> 17; range plus extra conjunct 42 -> 17; `ts >= X` tail 31 -> 19; 40% range with key output 37 -> 27; unfiltered/wide/unsorted-key controls within noise. Outputs identical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`ts > 4000000.5` on an Int column fell to decoding whole predicate columns, because only Int and Date literals were absorbed. A Double literal against an Int column is now folded into the interval as the integer bound it is equivalent to (`> 5.5` is `>= 6`, `<= 5.5` is `<= 5`, a fractional `==` is an empty interval that prunes every group). The translation is exact below 2^53: the derived bounds are representable and rounding an int64 to double is monotone, so comparing the column with the literal agrees with comparing it with the bound whether the comparison is exact or goes through double. Non-finite literals and magnitudes at or above 2^53 stay ordinary conjuncts, as does a Double literal against a Date column. Release, 10M rows, 8 cores, interleaved A/B, min of 7 (ms, before -> after): 1% range 34 -> 16; 40% range with key output 36 -> 29; one-sided tail 31 -> 16; fractional equality 32 -> 11. Integer-literal and unfiltered controls unchanged; outputs identical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A bare empty field in an otherwise-numeric column previously failed the int64/double inference probes outright, demoting the whole column to String (and, downstream, making count() include the blank as a value and sum() fail) even when no null spec was passed. The probes now treat an empty field as null when the column resolves to a numeric type, using a local copy of the validity bitmap so the empty-as-null bits never leak into a column that ends up String instead (where empty fields keep reading as "" as before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Merged locally |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
No description provided.