fix: classify source-free aggregate calls correctly (COUNT(*)) - #13
Draft
eitsupi wants to merge 3 commits into
Draft
fix: classify source-free aggregate calls correctly (COUNT(*))#13eitsupi wants to merge 3 commits into
eitsupi wants to merge 3 commits into
Conversation
The order of `ColumnLineage.mappings` varied between runs of the same
binary whenever a query had duplicate output column names:
SELECT a.id, b.id FROM a JOIN b ON a.id = b.bid
sometimes [ id <- a.id , id <- b.id ]
sometimes [ id <- b.id , id <- a.id ]
With three duplicates, four distinct orderings showed up across six runs.
Callers that cache or diff results see the same input produce different
output.
`resolve` collected the output nodes into a `HashSet<NodeId>` and built
the mappings by iterating it, so construction order followed hash order
with a per-process random seed. The sort afterwards could not undo this:
it keyed on a `HashMap<String, usize>` of output names, so duplicate
names collided and one index won, leaving same-named mappings in
whatever order the set had produced.
`ordered_cols` already holds the projection order, so build the mappings
straight from it. That makes the sort redundant — it can only reproduce
the order the loop now has, and it is the reason duplicates were
reordered in the first place — so it goes too.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The resolver returned ColumnOrigin::Concrete with invented table names
("?unknown?", "?cte?") when a column could not be resolved, so callers
could not tell fabricated lineage from real lineage.
Both sites now return ColumnOrigin::Ambiguous with an empty candidate
list, which already means "catalog needed to resolve" and keeps the
0.2.x public API unchanged. Returning None instead would drop the source
from the mapping entirely, and an empty source list already has a valid
meaning (constant expressions).
apply_catalog now skips empty candidate lists. Without that guard a
CatalogProvider resolving a column by name alone would turn an
unresolved origin straight back into a fabricated Concrete one.
determine_edge_kind correctly identifies any aggregate function call as EdgeKind::ViaAggregation regardless of its arguments, but that kind was only ever attached to the graph as an edge to an ancestor column. COUNT(*) has no column ancestor (`*` is a FunctionArgExpr::Wildcard, not an Expr, so collect_ancestors never visits it), so the correctly computed kind was silently discarded and the output's transform classification fell back to Direct — indistinguishable from a literal constant. Store the defining expression's intrinsic edge kind on the Output node itself, and use it as a fallback in derive_transform whenever no ancestor edge exists to classify from. This generalizes to any zero-ancestor aggregate/conditional/expression, not just COUNT(*).
eitsupi
marked this pull request as draft
August 23, 2026 03:14
eitsupi
marked this pull request as ready for review
August 23, 2026 03:15
eitsupi
marked this pull request as draft
August 23, 2026 03:16
This was referenced Aug 23, 2026
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.
This PR is based on #12.
Note
This change was written by Claude Code and Codex under my review and direction.
I have verified the diff, run the test suite, and confirmed
the behavior change against a probe crate. Please review it as you would any other
patch. Happy to split this into two PRs if you prefer one fix per PR — the two
commits are independent and can be cherry-picked separately.
Summary
determine_edge_kindalready correctly identifies any aggregate function call (COUNT,SUM,MAX, etc.) asEdgeKind::ViaAggregationregardless of its arguments — it only inspects the function name. But that kind is only ever attached to the lineage graph as an edge to an ancestor column:COUNT(*)has no column ancestor —*inside a function call isFunctionArgExpr::Wildcard, notFunctionArgExpr::Expr, socollect_ancestorsnever visits it andancestorsis empty. Theforloop body never runs, so the correctly-computedkindis silently discarded. Later,derive_transformclassifies the output purely from edges that exist in the graph; with zero edges it falls back toTransformKind::Direct— indistinguishable from a genuine literal constant likeSELECT 1 AS c.Fix
Store the defining expression's intrinsic edge kind on the
Outputgraph node itself (computed at build time, where the AST is visible), and use it as a fallback inderive_transformwhenever no ancestor edge exists to classify from. This generalizes beyondCOUNT(*)to any zero-ancestor aggregate/conditional/expression, not just this one function.sourcesstaying empty forCOUNT(*)is unchanged and correct — there genuinely is no column source. Only the transform classification changes.Tests
select_count_star_is_aggregation_without_sources, modeled on the existingselect_aggregatetest, assertingCOUNT(*)reportstransform: Aggregationwith emptysources.