ORCA: align CBitSet vec_size for grouping-set bitsets#1754
Open
yjhjstz wants to merge 2 commits into
Open
Conversation
yjhjstz
force-pushed
the
orca/grouping-sets-bitset-vec-size
branch
from
ae9752e to
6010360
Compare
leborchuk
reviewed
May 20, 2026
yjhjstz
added a commit
to yjhjstz/cloudberry
that referenced
this pull request
May 20, 2026
Address review feedback on apache#1754: - Add GPOS_ASSERT in CBitSet::Union/Intersection that destination and source share m_vector_size. The bug fixed in 6010360 was a silent miscalculation: Union splices in the source's links verbatim, but FindLinkByOffset on the destination uses our own m_vector_size to compute offsets, so any link past the smaller vec_size becomes unreachable from Get() while still walkable by CBitSetIter. Catch this class of misuse in debug builds rather than letting it surface later as wrong results. - Add a cube() variant of the rollup() repro to groupingsets.sql. Before the fix, cube(a, ab) also returned 0 rows for the same reason; this guards against regressions in the cube path.
yjhjstz
force-pushed
the
orca/grouping-sets-bitset-vec-size
branch
from
6cf1c77 to
db8c4e9
Compare
CreateGroupingSetsForRollup / Cube and the GROUPING_SET_EMPTY case in
GetColumnAttnosForGroupBy were constructing their accumulator/seed
CBitSets via the default ctor (vec_size = 256), then Union'ing in
per-grouping-set bitsets built with vec_size = num_cols. CBitSet::Union
just splices in any missing CBitSetLinks wholesale, so the accumulator
ended up with a link at offset 0 (vec_size 256) plus a stray link at
offset num_cols (vec_size num_cols) covering the high tleSortGroupRef.
CBitSet::Get then computed the offset using the destination's
m_vector_size = 256 and never consulted the stray link, while
CBitSetIter happily walked both -- so Get(k) disagreed with the iterator
for any k >= num_cols.
In CreateDXLProjectNullsForGroupingSets this caused tleSortGroupRefs >=
num_cols to be misclassified as non-grouping columns and NULL'd out,
even in grouping sets that included them. Visible as:
select generate_series(1, a) g, a+b ab
from (values (1,1),(2,2)) t(a,b)
group by rollup(a, ab) order by 1,2;
returning 0 rows instead of 6 -- the rollup(a, ab) branch projected the
a column as NULL, so generate_series(1, NULL) produced no rows.
Fix by passing num_cols when constructing the accumulator and seed
bitsets so all participants in the Union share m_vector_size.
Add the repro to groupingsets.sql.
yjhjstz
force-pushed
the
orca/grouping-sets-bitset-vec-size
branch
from
db8c4e9 to
db6a19f
Compare
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
2 participants
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.
CreateGroupingSetsForRollup / Cube and the GROUPING_SET_EMPTY case in GetColumnAttnosForGroupBy were constructing their accumulator/seed CBitSets via the default ctor (vec_size = 256), then Union'ing in per-grouping-set bitsets built with vec_size = num_cols. CBitSet::Union just splices in any missing CBitSetLinks wholesale, so the accumulator ended up with a link at offset 0 (vec_size 256) plus a stray link at offset num_cols (vec_size num_cols) covering the high tleSortGroupRef. CBitSet::Get then computed the offset using the destination's m_vector_size = 256 and never consulted the stray link, while CBitSetIter happily walked both -- so Get(k) disagreed with the iterator for any k >= num_cols.
In CreateDXLProjectNullsForGroupingSets this caused tleSortGroupRefs >= num_cols to be misclassified as non-grouping columns and NULL'd out, even in grouping sets that included them. Visible as:
select generate_series(1, a) g, a+b ab
from (values (1,1),(2,2)) t(a,b)
group by rollup(a, ab) order by 1,2;
returning 0 rows instead of 6 -- the rollup(a, ab) branch projected the a column as NULL, so generate_series(1, NULL) produced no rows.
Fix by passing num_cols when constructing the accumulator and seed bitsets so all participants in the Union share m_vector_size.
Add the repro to groupingsets.sql.
Fixes #1753
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions