Repository navigation
Fix shared-subplan merges across outer joins and limits, and cuDF string MIN/MAX beside NUNIQUE - #11
Merged
Merged
Conversation
The shared_subplan_materialization rule treats a grouped branch's reader predicate as post-aggregate filterable when the group column is a source column carried unchanged to the aggregate. materialized_column_lineage followed a join output to its input regardless of join type, so a predicate on the null-supplying input of an outer join was re-applied after the shared aggregate and dropped the NULL group each branch keeps. It also followed a Limit, so two LIMIT branches over different predicates were merged into one limit over the widened rows, which keeps a different prefix. JoinType::passes_through_rows_of names the inputs a join passes through unchanged (both for inner and cross, the left for left outer, semi, anti and mark, neither for full). Lineage now stops at a join output from any other input and at a Limit. TPC-H and TPC-DS applied merges are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…regations store_result_functor::get_grouped_values() returned the cached within-group sorted values once NUNIQUE, MEDIAN or a quantile had built them. ARGMIN and ARGMAX map a position in the grouped values back to a row through the key sort order, and NTH_ELEMENT and COLLECT_LIST read the rows of a group in that order, so they received the wrong rows: MIN and MAX of a string column returned the group's first and last row in key order. Grouped values are now always the values gathered by the key sort order; the scan override that existed to avoid the substitution is removed. Found by a DuckDB cross-check of an Algeon query combining COUNT(DISTINCT x), MIN(x) and median(y) over an Open Targets table.
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.
Two correctness fixes found while running Algeon on public science datasets, taken from the science-loop work without its demo datasets, recorder or blog post. One commit per fix.
Correctness
97ea333d6):shared_subplan_materializationfollowed column lineage through any join and through aLIMIT. A predicate on the null-supplying side of an outer join was re-applied after the shared aggregate and dropped theNULLgroup each branch keeps. TwoLIMITbranches with different predicates merged into one limit over the widened rows, which keeps a different prefix.JoinType::passes_through_rows_ofnow names the inputs a join passes through unchanged, and lineage stops at any other join input and at a limit. TPC-H and TPC-DS applied merges are unchanged.e85a3fe0e): in the cuDF fork's sort-based groupby, NUNIQUE, MEDIAN or a quantile left within-group sorted values cached. ARGMIN, ARGMAX, NTH_ELEMENT and COLLECT_LIST on the same column then read those values instead of the key-ordered ones, soMIN/MAXof a string column returned the group's first or last row in key order. The fork keeps grouped values in key sort order (algeon-dev/cudfalgeon-26.08at4969938047; this PR moves the submodule to it). A DuckDB cross-check of aCOUNT(DISTINCT x), MIN(x), median(y)query over an Open Targets table found it.website/docs/reference/forks.mdlists the fix.Validation
algeon-query-planning, and the cuDFsemantic_contracts_gpubinary (groupby_casesincludes the newgroupby_string_min_max_beside_nunique_on_same_column). 198 + 22 tests pass. The installed libcudf is built from the fixed fork source.--all-features -D warnings), nightly fmt and the prose check pass. The full pre-merge gate was not run.Not in this PR: the demo datasets, the science-loop recorder and blog post, the dataset-table version of the third-party data check, and the
bitwise_hammingmetric forcuvs_cagra.🤖 Generated with Claude Code