HIVE-29652:Improve range selectivity estimations in the absence of histograms - #6746
HIVE-29652:Improve range selectivity estimations in the absence of histograms#6746Manya0407 wants to merge 1 commit into
Conversation
|
thomasrebele
left a comment
thomasrebele
left a comment
There was a problem hiding this comment.
Thank you for the PR, @Manya0407! I've left some suggestions how the code could be simplified. There are also some unexpected test results to be discussed.
| case TIMESTAMP: | ||
| min = range.minValue.longValue(); | ||
| max = range.maxValue.longValue(); | ||
| break; | ||
| case TINYINT: | ||
| min = range.minValue.byteValue(); | ||
| max = range.maxValue.byteValue(); | ||
| break; | ||
| case SMALLINT: | ||
| min = range.minValue.shortValue(); | ||
| max = range.maxValue.shortValue(); | ||
| break; | ||
| case INTEGER: | ||
| min = range.minValue.intValue(); | ||
| max = range.maxValue.intValue(); | ||
| break; | ||
| case BIGINT: | ||
| min = range.minValue.longValue(); | ||
| max = range.maxValue.longValue(); | ||
| break; | ||
| case FLOAT: | ||
| min = range.minValue.floatValue(); | ||
| max = range.maxValue.floatValue(); | ||
| break; | ||
| case DOUBLE: | ||
| min = (float) range.minValue.doubleValue(); | ||
| max = (float) range.maxValue.doubleValue(); | ||
| break; |
There was a problem hiding this comment.
Couldn't we combine these cases by using java.lang.Number#floatValue?
We could even change it to java.lang.Number#doubleValue. The method FilterSelectivityEstimator#extractLiteral(org.apache.calcite.rex.RexNode) returns float because the histogram stores float values. The range and boundary type could be changed to Double as well. Maybe this is out-of-scope for HIVE-29652, as it would require a bit of refactoring to not lose information.
| return (RexLiteral) REX_BUILDER.makeLiteral(calendar, | ||
| REX_BUILDER.getTypeFactory().createSqlType(SqlTypeName.DATE), true); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we could use org.apache.calcite.rex.RexBuilder#makeDateLiteral here.
| default: | ||
| return Optional.empty(); | ||
| } | ||
| return Optional.of(new float[] { min, max }); |
There was a problem hiding this comment.
Please return a Optional<Range<...>>.
| if (!left.isConnected(right)) { | ||
| return null; | ||
| } | ||
| Range<Float> intersection = left.intersection(right); | ||
| if (intersection.isEmpty()) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Please return a valid range. If the Range is empty, then return Range.closedOpen(0f, 0f). The callers that check for null can then be simplified.
| return lowerOk && upperOk; | ||
| } | ||
|
|
||
| private static Range<Float> intersectClosedOpenRanges(Range<Float> left, Range<Float> right) { |
There was a problem hiding this comment.
How about calling this method "intersectRanges"? I don't see why it should be limited to closed-open ranges.
| return overlapWidth / domainWidth; | ||
| } | ||
|
|
||
| private static boolean isPointInClosedRange(Range<Float> boundaries, float point) { |
There was a problem hiding this comment.
Please use com.google.common.collect.Range#contains.
| } | ||
|
|
||
| private Double computeUniformRangeSelectivity(ColStatistics cs, Range<Float> boundaries, HiveTableScan scan, | ||
| boolean inverseBool, Range<Float> typeRange, RelDataType columnType) { |
There was a problem hiding this comment.
I have the feeling that this method could be simplified by using the approach of computeTwoSidedUniformSelectivity (intersect(intersect(minMaxRange, typeRange), boundaries)) also for one-sided predicates. Could you try that, please?
| RexNode int50 = REX_BUILDER.makeLiteral(50, TYPE_FACTORY.createSqlType(INTEGER), true); | ||
| RexNode filter = REX_BUILDER.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, currentInputRef, int50); | ||
| FilterSelectivityEstimator estimator = new FilterSelectivityEstimator(scan, mq); | ||
| Assert.assertEquals(0.5, estimator.estimateSelectivity(filter), DELTA); |
There was a problem hiding this comment.
Shouldn't the expected result be 51/101? There are 101 integers in the interval [0, 100] and 51 integers from that interval that fulfill the predicate x <= 50, i.e., [0, 50]. So I would have expected the selectivity to be 51/101 or about 0.5049505.
| RexNode filter = REX_BUILDER.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, currentInputRef, | ||
| literalDate("2020-11-04")); | ||
| FilterSelectivityEstimator estimator = new FilterSelectivityEstimator(scan, mq); | ||
| Assert.assertEquals(0.5, estimator.estimateSelectivity(filter), DELTA); |
There was a problem hiding this comment.
Similar to https://github.com/apache/hive/pull/6746/changes#r3903821835, I would have expected a selectivity of 4/7 or 0.5714286.
3 participants
What changes were proposed in this pull request?
Adds MIN/MAX uniform-distribution selectivity estimation to FilterSelectivityEstimator when histograms are absent. The hierarchy is now: histogram → MIN/MAX (if hive.stats.filter.range.uniform=true) → existing 1/3 / 1/NDV fallback. Also wires the config through HiveConfPlannerContext/CalcitePlanner and adds unit tests including DATE days→seconds handling.
Why are the changes needed?
Without histograms, CBO currently uses hardcoded selectivity (1/3, 1/NDV) even when MIN/MAX stats exist, hurting cardinality estimates and plan quality. The annotation path already uses MIN/MAX uniform estimation; this aligns the CBO path with that behavior.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added unit tests in TestFilterSelectivityEstimator for comparisons, BETWEEN/NOT BETWEEN, boundaries, nulls, config off, SEARCH, and DATE conversion. Ran mvn -pl ql -Dtest=TestFilterSelectivityEstimator test