Copilot commented on code in PR #6505:
URL: https://github.com/apache/hive/pull/6505#discussion_r4461508318
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -558,18 +558,25 @@ private long evaluateInExpr(Statistics stats,
ExprNodeDesc pred, long currNumRow
factor *= children.size() - 1;
}
for (int i = 0; i < columnStats.size(); i++) {
- long dvs = columnStats.get(i) == null ? 0 :
columnStats.get(i).getCountDistint();
+ ColStatistics cs = columnStats.get(i);
+ long dvs = cs == null ? -1L : cs.getCountDistint();
if (dvs == 0) {
- factor *= 0.5;
- continue;
+ // verified zero distinct values: IN cannot match any row
+ factor = 0;
+ break;
Review Comment:
Setting `factor` to 0 does not preserve the verified-zero estimate when
`hive.stats.filter.in.min.ratio` is positive. For stats without a min/max
range, such as an all-NULL string column, `allColsFilteredByStats` is false and
the later minimum-ratio clamp raises it again (1,000 rows with a 0.1 minimum
become 100 estimated matches). Return 0 immediately so a verified empty domain
bypasses the fallback for uncertain statistics.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -1381,6 +1384,16 @@ private long evaluateChildExpr(Statistics stats,
ExprNodeDesc child,
return numRows / 2;
}
+ private static long rowsAfterEqualityFilter(long numRows, long dvs) {
+ if (dvs < 0) {
+ return numRows / 2;
+ }
+ if (dvs == 0) {
+ return 0;
Review Comment:
`GenericUDFOPEqualNS` extends `GenericUDFOPEqual`, so null-safe comparisons
also reach this helper. An all-NULL column has NDV 0, but `col IS NOT DISTINCT
FROM NULL` matches every row; returning zero therefore severely underestimates
this filter. Handle null-safe equality against NULL using `numNulls` before the
NDV-only helper, and cover both operand orders.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -1820,9 +1835,24 @@ private boolean checkMapSideAggregation(GroupByOperator
gop,
// estimate size of key from column statistics
long avgKeySize = 0;
+ // lazily computed on first unknown NDV (null = not yet looked up)
+ Long parentNumRows = null;
for (ColStatistics cs : colStats) {
if (cs != null) {
- numEstimatedRows = StatsUtils.safeMult(numEstimatedRows,
cs.getCountDistint());
+ long ndv = cs.getCountDistint();
+ if (ndv < 0) {
+ if (parentNumRows == null) {
+ // unknown NDV: fall back to parentNumRows / 2, matching the
heuristic
+ // used elsewhere in this file when GROUP BY cardinality
cannot be computed
+ Statistics parentStats =
gop.getParentOperators().get(0).getStatistics();
+ parentNumRows = (parentStats != null) ?
parentStats.getNumRows() : -1L;
+ }
+ if (parentNumRows <= 0) {
+ return false;
+ }
+ ndv = parentNumRows / 2;
+ }
+ numEstimatedRows = StatsUtils.safeMult(numEstimatedRows, ndv);
Review Comment:
Applying `parentNumRows / 2` separately to every unknown key estimates
25,000,000 groups for 10,000 rows and two unknown-NDV keys. Without grouping
sets, at most 10,000 groups can exist, so the inflated memory estimate can
incorrectly send cardinality estimation through the no-hash-aggregation branch.
Cap the combined estimate at the input-row count, accounting for grouping-set
expansion when present.
--
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]
