zhangwenchao-123 commented on code in PR #1599:
URL: https://github.com/apache/cloudberry/pull/1599#discussion_r2909750821
##########
src/backend/commands/analyzeutils.c:
##########
@@ -759,17 +759,32 @@ getNdvBySegHeapTuple(AttStatsSlot * *ndvbsSlots,
HeapTuple *heaptupleStats, floa
(void) get_attstatsslot(ndvbsSlots[i], heaptupleStats[i],
STATISTIC_KIND_NDV_BY_SEGMENTS, InvalidOid,
ATTSTATSSLOT_VALUES);
- if ((InvalidOid != ndvbsSlots[i]->valuetype && // result is not
empty
- // not empty partition with invalid ndvbs
- (relTuples[i] > 0 &&
DatumGetFloat8(ndvbsSlots[i]->values[0]) == 0)) ||
- // not empty partition without ndvbs
- (InvalidOid == ndvbsSlots[i]->valuetype && relTuples[i]
> 0)) {
- valid = false;
- break;
+ if (ndvbsSlots[i]->valuetype != FLOAT8OID)
+ {
+ /*
+ * NDV_BY_SEGMENTS slot not found or has unexpected
type.
+ * Non-empty partitions must have valid NDV_BY_SEGMENTS;
+ * empty partitions (relTuples == 0) can be skipped.
+ */
+ if (relTuples[i] > 0)
+ {
+ valid = false;
+ break;
+ }
+ free_attstatsslot(ndvbsSlots[i]);
+ pfree(ndvbsSlots[i]);
+ ndvbsSlots[i] = NULL;
+ continue;
}
- Assert(ndvbsSlots[i]->valuetype == FLOAT8OID);
Review Comment:
Good point. Added back `Assert(ndvbsSlots[i]->valuetype == FLOAT8OID)` after
the type check block. Also changed `Assert(ndvbsSlots[i]->nvalues == 1)` to a
runtime check as yjhjstz suggested.
##########
src/backend/commands/analyzeutils.c:
##########
@@ -759,17 +759,32 @@ getNdvBySegHeapTuple(AttStatsSlot * *ndvbsSlots,
HeapTuple *heaptupleStats, floa
(void) get_attstatsslot(ndvbsSlots[i], heaptupleStats[i],
STATISTIC_KIND_NDV_BY_SEGMENTS, InvalidOid,
ATTSTATSSLOT_VALUES);
- if ((InvalidOid != ndvbsSlots[i]->valuetype && // result is not
empty
- // not empty partition with invalid ndvbs
- (relTuples[i] > 0 &&
DatumGetFloat8(ndvbsSlots[i]->values[0]) == 0)) ||
- // not empty partition without ndvbs
- (InvalidOid == ndvbsSlots[i]->valuetype && relTuples[i]
> 0)) {
- valid = false;
- break;
+ if (ndvbsSlots[i]->valuetype != FLOAT8OID)
+ {
+ /*
+ * NDV_BY_SEGMENTS slot not found or has unexpected
type.
+ * Non-empty partitions must have valid NDV_BY_SEGMENTS;
+ * empty partitions (relTuples == 0) can be skipped.
+ */
+ if (relTuples[i] > 0)
+ {
+ valid = false;
+ break;
+ }
+ free_attstatsslot(ndvbsSlots[i]);
+ pfree(ndvbsSlots[i]);
+ ndvbsSlots[i] = NULL;
+ continue;
}
- Assert(ndvbsSlots[i]->valuetype == FLOAT8OID);
Assert(ndvbsSlots[i]->nvalues == 1);
+
+ /* Non-empty partition with zero NDV is suspicious */
+ if (relTuples[i] > 0 &&
DatumGetFloat8(ndvbsSlots[i]->values[0]) == 0)
+ {
+ valid = false;
+ break;
+ }
}
return valid;
Review Comment:
Right, `aggregate_leaf_partition_ndvbs` will free all ndvbsSlots regardless
of validity. The early cleanup here for empty partitions (setting slot to NULL
after free) is just to keep the slot array consistent during iteration — it
won't cause a double-free since the cleanup function checks for NULL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
