HIVE-29479: Improve histogram-based selectivity estimation for two-sided range predicates#6477
HIVE-29479: Improve histogram-based selectivity estimation for two-sided range predicates#6477rubenada wants to merge 9 commits into
Conversation
…ded range predicates
rubenada
changed the title
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
soumyakanti3578
left a comment
soumyakanti3578
left a comment
There was a problem hiding this comment.
Overall looks good to me, but maybe we should add tests for IS [NOT] NULL with SEARCH, and also a couple of tests for SEARCH containing both ranges and points?
|
Please go through the Sonar issues as well. I think some of them are good suggestions. :) |
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
thomasrebele
left a comment
thomasrebele
left a comment
There was a problem hiding this comment.
Thank you for the PR! The overall approach looks good. I've made a few suggestions and requests.
| selectivityList.add(rexBuilder.makeCall(HiveIn.INSTANCE, operands).accept(FilterSelectivityEstimator.this)); | ||
| } | ||
|
|
||
| return selectivityList.size() == 1 ? selectivityList.get(0) : computeDisjunctionSelectivity(selectivityList); |
There was a problem hiding this comment.
As far as I understand computeDisjunctionSelectivity, it combines the selectivities by multiplying their complements. To use the information from the histogram, we would need to add the selectivities (but making sure we stay within the possible values [0,1]).
I wonder how accurate estimating selectivity of individual values are compared to the ranges. This also depends on the type of histogram (I had created a ticket about the shortcoming of KLL) . I think we can just add the selectivities for individual values and ranges together, and revisit this in the future in case of need.
There was a problem hiding this comment.
Indeed. I have applied the change to add the ranges selectivities (and it gives better results in certain tests, as expected).
I have kept the "disjunction logic" for the combination with the other expressions (EQ/IN, IS_NULL), since it seems more aligned with how this class works in general when computing OR-combined RexNodes. But I agree this can be revisits in the future.
| rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, ref).accept(FilterSelectivityEstimator.this)); | ||
| } | ||
|
|
||
| RangeSets.forEach(sarg.rangeSet, new RangeSets.Consumer<C>() { |
There was a problem hiding this comment.
Maybe the code could be simplified with for (Range<C> range : sarg.rangeSet.asRanges()) { ... }? It might be possible to treat a range as-is, without differentiating all the different kinds of ranges.
There was a problem hiding this comment.
Good point! I have applied the suggested refactoring.
There was a problem hiding this comment.
While I agree that the refactored code is much smaller/simplified now, I feel the previous version was more organized and readable as it's now a huge method with multiple if else blocks.
Moreover, I see that implementing RangeSets.Consumer<C> is the preferred method both in Hive (org.apache.hadoop.hive.ql.optimizer.calcite.RangeConverter) and in Calcite (several places). If the new code is not significantly more performant than the earlier version, then maybe we should keep things familiar?
Another small benefit of implementing RangeSets.Consumer<C> is it will be easily searchable from IDE by looking for all subclasses.
BTW, I am willing to approve this as-is, but just wanted to hear both of your thoughts on this.
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
|
@soumyakanti3578 , @thomasrebele thanks for the review. I think I have addressed all the remarks, feel free to take another look. |
4 participants
See HIVE-29479.
What changes were proposed in this pull request?
This PR adapts FilterSelectivityEstimator so that histogram statistics are used for all types of range predicates (so far it was only done for single-sided range predicates and bounded ranges).
Why are the changes needed?
This PR allows the CBO planner to use histogram statistics for all types of range predicates.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unit tests added.