fix: harden column lineage resolution and performance - #14
Draft
eitsupi wants to merge 11 commits into
Draft
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
changed the title
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
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.
Summary
This stacked change hardens column-lineage resolution across catalog expansion and nested scopes:
base.items[OFFSET(0)]tracesbase.itemsinstead of fabricating abasecolumnThe CompoundFieldAccess implementation is structural and dialect-independent. The BigQuery
OFFSET(0)case is included as a dialect-specific parser regression test, not as a special-case code path.API compatibility
This PR does not change public Rust types or function signatures. It contains intentional behavioral corrections:
Performance
Scope mappings are cached within one resolution run. This keeps named-column lookups from recursively rebuilding every output mapping of the referenced scope. The cache remains internal and does not change catalog or output semantics.
Stack
This PR is stacked on the fork history containing the changes submitted in #12 and #13. It can be rebased or retargeted as the lower changes land upstream.
A separate API branch used by dlin is rebased on this PR. The commits in this PR itself remain API-invariant so they can be reviewed and integrated independently.
Verification
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all --checkRegression coverage includes nested/unknown set-operation shapes, projection scopes, memoization reuse, qualified alias stars, table/CTE/unqualified CompoundFieldAccess, nested fields, column-dependent subscripts, and BigQuery
OFFSET(0).