asolimando commented on code in PR #3137:
URL: https://github.com/apache/hive/pull/3137#discussion_r1043447078
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -834,6 +844,36 @@ private long evaluateBetweenExpr(Statistics stats,
ExprNodeDesc pred, long currN
return currNumRows;
}
+ try {
+ if (comparisonExpression instanceof ExprNodeColumnDesc) {
+ final ExprNodeColumnDesc columnDesc = (ExprNodeColumnDesc)
comparisonExpression;
+ ColStatistics cs =
stats.getColumnStatisticsFromColName(columnDesc.getColumn());
+ if (FilterSelectivityEstimator.isHistogramAvailable(cs)) {
+ final KllFloatsSketch kll =
KllFloatsSketch.heapify(Memory.wrap(cs.getHistogram()));
+ final String colTypeLowerCase =
columnDesc.getTypeString().toLowerCase();
+ final String leftValueString = leftExpression instanceof
ExprNodeConstantDesc
+ ? ((ExprNodeConstantDesc)
leftExpression).getValue().toString() : leftExpression.getExprString();
+ final String rightValueString = rightExpression instanceof
ExprNodeConstantDesc
+ ? ((ExprNodeConstantDesc)
rightExpression).getValue().toString() : rightExpression.getExprString();
+ final float leftValue =
extractFloatFromLiteralValue(colTypeLowerCase, leftValueString);
+ final float rightValue =
extractFloatFromLiteralValue(colTypeLowerCase, rightValueString);
+ if (invert) {
+ // column < leftValue OR column > rightValue
+ if (rightValue < leftValue) {
+ return kll.getN();
+ }
+ return Math.round(kll.getN() * (lessThanSelectivity(kll,
leftValue) + greaterThanSelectivity(kll, rightValue)));
+ }
+ // if they are equal we can't handle it here, it becomes an
equality predicate
+ if (Float.compare(leftValue, rightValue) != 0) {
+ return Math.round(kll.getN() *
FilterSelectivityEstimator.betweenSelectivity(kll, leftValue, rightValue));
+ }
+ }
+ }
+ } catch(IllegalArgumentException e) {
Review Comment:
In theory `Timestamp.valueOf()` and `Date.valueOf()` only generates
`IllegalArgumentException`, while all the numeric data types throws
`NumberFormatException` which extends `IllegalArgumentException`.
However, `Float.parseFloat()`, `Double.parseDouble()`, `new BigDecimal()`,
`Timestamp.valueOf()` and `Date.valueOf()` throw an `NullPointerException` when
the input string is `null`, we can check for that and throw an
`IllegalArgumentException`.
Since as a safety net we can always resort to the standard computation, I
have nothing against catching a more general exception, but in that case I
think it's better `catch (RuntimeException e) {...}` so we don't catch and
ignore stuff like `InterruptedException` which would be pretty bad.
Anyway, if they are not in the method signature, they must inherit from
`RuntimeException`, so I think it' safe.
I will update the unit tests to cover this case too.
EDIT: finally I think it's better to catch `RuntimeException` which covers
both `IllegalArgumentException` and `NullPointerException`, I have added some
debug logs saying what's happening and updated unit tests to reflect the change.
--
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]
