Skip to content

fix: classify source-free aggregate calls correctly (COUNT(*)) - #13

Draft
eitsupi wants to merge 3 commits into
funcpp:mainfrom
eitsupi:fix/count-star-aggregation-classification
Draft

fix: classify source-free aggregate calls correctly (COUNT(*))#13
eitsupi wants to merge 3 commits into
funcpp:mainfrom
eitsupi:fix/count-star-aggregation-classification

Conversation

@eitsupi

@eitsupi eitsupi commented Aug 23, 2026

Copy link
Copy Markdown

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_kind already correctly identifies any aggregate function call (COUNT, SUM, MAX, etc.) as EdgeKind::ViaAggregation regardless 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:

let kind = determine_edge_kind(expr);
let output = self.graph.add_output(name.clone());
for &anc in &ancestors {
    self.graph.add_edge(anc, output, kind.clone());
}

COUNT(*) has no column ancestor — * inside a function call is FunctionArgExpr::Wildcard, not FunctionArgExpr::Expr, so collect_ancestors never visits it and ancestors is empty. The for loop body never runs, so the correctly-computed kind is silently discarded. Later, derive_transform classifies the output purely from edges that exist in the graph; with zero edges it falls back to TransformKind::Direct — indistinguishable from a genuine literal constant like SELECT 1 AS c.

SELECT COUNT(*) AS c FROM t   -> transform: Direct   (wrong)
SELECT COUNT(x) AS c FROM t   -> transform: Aggregation (correct — has a column ancestor)

Fix

Store the defining expression's intrinsic edge kind on the Output graph node itself (computed at build time, where the AST is visible), and use it as a fallback in derive_transform whenever no ancestor edge exists to classify from. This generalizes beyond COUNT(*) to any zero-ancestor aggregate/conditional/expression, not just this one function.

sources staying empty for COUNT(*) is unchanged and correct — there genuinely is no column source. Only the transform classification changes.

Tests

  • Full existing test suite passes unmodified (94 tests across the crate).
  • Added select_count_star_is_aggregation_without_sources, modeled on the existing select_aggregate test, asserting COUNT(*) reports transform: Aggregation with empty sources.
  • Confirmed the new test fails on the pre-fix code (reverted the implementation, re-ran) and passes with the fix.

eitsupi and others added 3 commits August 22, 2026 01:24
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
eitsupi marked this pull request as draft August 23, 2026 03:14
@eitsupi
eitsupi marked this pull request as ready for review August 23, 2026 03:15
@eitsupi
eitsupi marked this pull request as draft August 23, 2026 03:16
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