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 7 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.
|
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.