Repository navigation
Conversation
yjhjstz
reviewed
Sep 1, 2026
roseduan
force-pushed
the
fix_orca_nullable_unqiue_index_agg
branch
2 times, most recently
from
October 19, 2026 03:46
aada5be to
6cf597e
Compare
CXformSimplifyGbAgg::FDropGbAgg() drops a GROUP BY/DISTINCT's
aggregate node when the grouping columns cover a key derived from
a UNIQUE or PRIMARY KEY constraint, assuming the key already
guarantees one row per value.
That's wrong for a plain UNIQUE column: SQL allows multiple NULLs
in it (NULL is never "equal" for uniqueness checks), but dedup
must still collapse those NULLs into one row.
Before:
create table repro (c0 numeric unique);
insert into repro values (1), (2), (null), (null), (null);
explain (costs off) select distinct c0 from repro;
Gather Motion 3:1 (slice1; segments: 3)
-> Seq Scan on repro -- no aggregate, all 5 rows returned
After:
Gather Motion 3:1 (slice1; segments: 3)
-> GroupAggregate
Group Key: repro.c0
-> Seq Scan on repro -- correctly returns 1, 2, NULL
Fix by also requiring every key column to be provably NOT NULL
(CExpression::DeriveNotNullColumns()) before dropping the
aggregate. PRIMARY KEY columns are always NOT NULL, so that case
is unaffected.
roseduan
force-pushed
the
fix_orca_nullable_unqiue_index_agg
branch
from
October 19, 2026 08:19
eb521c3 to
be750a1
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
PdrgpcrKey(mp)does not guarantee it returns the key at position 0, butFNullsDistinct(0)… · NewPdrgpcrKey(mp)does not guarantee it returns the key at position 0, butFNullsDistinct(0)… · NewFNullsDistinct()readsm_pbsNullsDistinctat an index without guarding thatul < Keys(). If… · NewCColRefTable::PcrConvert(colref)assumes every output colref here is a table colref. If that… · New
What changed in this PR
Fixes an optimizer correctness issue where ORCA could incorrectly drop a GROUP BY / DISTINCT aggregate when the grouping columns cover a UNIQUE-derived key that is nullable (allowing multiple NULLs), by tracking NULL-distinctness of keys and requiring NOT NULL proof before dropping the aggregate.
Changes:
- Track whether derived keys allow multiple NULLs (NULLs are distinct) in
CKeyCollection. - Update
CXformSimplifyGbAgg::FDropGbAgg()to only drop GbAgg when the key is not NULL-distinct or all key columns are provably NOT NULL. - Add regression coverage for DISTINCT on UNIQUE nullable columns (including partitioned tables) and update expected outputs.
| File | Description |
|---|---|
| src/test/regress/sql/bfv_aggregate.sql | Adds SQL regression cases for DISTINCT over nullable UNIQUE keys (incl. partitions). |
| src/test/regress/expected/bfv_aggregate_optimizer.out | Updates ORCA plan expectations to reflect preserved aggregates when NULLs matter. |
| src/test/regress/expected/bfv_aggregate.out | Updates planner (non-ORCA) expected output for new regression cases. |
| src/backend/gporca/libgpopt/src/xforms/CXformSimplifyGbAgg.cpp | Requires NOT NULL proof (or non-NULL-distinct key) before dropping GbAgg. |
| src/backend/gporca/libgpopt/src/operators/CLogical.cpp | Propagates NULL-distinctness when deriving/combining keys. |
| src/backend/gporca/libgpopt/src/base/CKeyCollection.cpp | Stores NULL-distinctness bitset per key; extends Add() behavior. |
| src/backend/gporca/libgpopt/include/gpopt/base/CKeyCollection.h | Adds bitset state and FNullsDistinct() accessor to CKeyCollection. |
Review effort: Lite
| CLogical::PkcCombineKeys(CMemoryPool *mp, CExpressionHandle &exprhdl) | ||
| { | ||
| CColRefSet *pcrs = GPOS_NEW(mp) CColRefSet(mp); | ||
| BOOL fNullsDistinct = false; |
Comment on lines
347
to
+352
| CColRefArray *colref_array = pkc->PdrgpcrKey(mp); | ||
| pcrs->Include(colref_array); | ||
| colref_array->Release(); | ||
|
|
||
| // the combined key permits multiple NULLs if one of its parts does | ||
| fNullsDistinct = fNullsDistinct || pkc->FNullsDistinct(0); |
Comment on lines
+62
to
+67
| // check if the key at given position permits multiple NULLs | ||
| BOOL | ||
| FNullsDistinct(ULONG ul) const | ||
| { | ||
| return m_pbsNullsDistinct->Get(ul); | ||
| } |
Comment on lines
+392
to
+397
| CColRef *colref = (*pdrgpcrOutput)[bsiter.Bit()]; | ||
| pcrs->Include(colref); | ||
|
|
||
| // a UNIQUE constraint permits multiple NULLs in nullable columns | ||
| fNullsDistinct = fNullsDistinct || | ||
| CColRefTable::PcrConvert(colref)->IsNullable(); |
Copilot stopped reviewing on behalf of
roseduan due to an error
October 20, 2026 03:04
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.
CXformSimplifyGbAgg::FDropGbAgg() drops a GROUP BY/DISTINCT's aggregate node when the grouping columns cover a key derived from a UNIQUE or PRIMARY KEY constraint, assuming the key already guarantees one row per value.
That's wrong for a plain UNIQUE column: SQL allows multiple NULLs in it (NULL is never "equal" for uniqueness checks), but dedup must still collapse those NULLs into one row.
Before:
create table repro (c0 numeric unique);
insert into repro values (1), (2), (null), (null), (null);
explain (costs off) select distinct c0 from repro;
Gather Motion 3:1 (slice1; segments: 3)
-> Seq Scan on repro -- no aggregate, all 5 rows returned
After:
Gather Motion 3:1 (slice1; segments: 3)
-> GroupAggregate
Group Key: repro.c0
-> Seq Scan on repro -- correctly returns 1, 2, NULL
Fix by also requiring every key column to be provably NOT NULL (CExpression::DeriveNotNullColumns()) before dropping the aggregate. PRIMARY KEY columns are always NOT NULL, so that case is unaffected.
Fixes #ISSUE_Number
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