You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/12/12 07:55:07 UTC
[GitHub] [hive] amansinha100 commented on a diff in pull request #3137: HIVE-26221: Add histogram-based column statistics
amansinha100 commented on code in PR #3137:
URL: https://github.com/apache/hive/pull/3137#discussion_r1045449998
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLPlanUtils.java:
##########
@@ -395,29 +404,54 @@ public void addDoubleStats(ColumnStatisticsData cd, List<String> ls) {
ls.add(lowValue + dc.getLowValue() + "'");
}
+ public String checkHistogram(ColumnStatisticsData cd) {
Review Comment:
nit: Strictly speaking, this is checking for the presence of the KLL sketch rather than the actual Histogram. The corresponding method for NDV is called checkBitVectors() instead of checkNDV(). At the same time, Histogram is more intuitive so I am ok with leaving this as-is and just add a comment for the method.
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -2824,6 +2824,12 @@ public static enum ConfVars {
"higher compute cost. (NDV means the number of distinct values.). It only affects the FM-Sketch \n" +
"(not the HLL algorithm which is the default), where it computes the number of necessary\n" +
" bitvectors to achieve the accuracy."),
+ HIVE_STATS_KLL_K("hive.stats.kll.k", 200,
Review Comment:
The single letter suffix 'k' comes across somewhat abrupt. Can it be something like 'kparameter' or 'kvalue' ?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -1004,41 +1045,51 @@ private long evaluateComparator(Statistics stats, AnnotateStatsProcCtx aspCtx, E
closedBound = isClosedBound(udf);
} else {
// default
- return numRows / 3;
+ return currNumRows / 3;
}
ColStatistics cs = stats.getColumnStatisticsFromColName(columnDesc.getColumn());
+ String colTypeLowerCase = columnDesc.getTypeString().toLowerCase();
+
+ if (FilterSelectivityEstimator.isHistogramAvailable(cs)) {
+ try {
+ return evaluateComparatorWithHistogram(
+ cs, currNumRows, colTypeLowerCase, boundValue, upperBound, closedBound);
+ } catch (SketchesArgumentException e) {
+ LOG.info("Sketch-based statistics estimation failed, falling back to regular estimation", e);
Review Comment:
nit: adding a delimiter such as ":" or just a whitespace between the log message and the exception helps in readability of the logs.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -1234,17 +1285,70 @@ private long evaluateComparator(Statistics stats, AnnotateStatsProcCtx aspCtx, E
// new estimate for the number of rows
return Math.round(
((maxValue.subtract(value)).divide(maxValue.subtract(minValue), RoundingMode.UP))
- .multiply(BigDecimal.valueOf(numRows))
+ .multiply(BigDecimal.valueOf(currNumRows))
.doubleValue());
}
}
}
} catch (NumberFormatException nfe) {
- return numRows / 3;
+ return currNumRows / 3;
}
}
// default
- return numRows / 3;
+ return currNumRows / 3;
+ }
+
+ private long evaluateComparatorWithHistogram(ColStatistics cs, long currNumRows, String colTypeLowerCase,
+ String boundValue, boolean upperBound, boolean closedBound) {
+ final KllFloatsSketch kll = KllFloatsSketch.heapify(Memory.wrap(cs.getHistogram()));
+
+ if (kll.getN() == 0) {
+ return 0;
+ }
+
+ try {
+ final float value = extractFloatFromLiteralValue(colTypeLowerCase, boundValue);
+
+ // kll ignores null values (i.e., kll.getN() + numNulls = currNumRows), we therefore need to use kll.getN()
+ // instead of currNumRows since the CDF is expressed as a fraction of kll.getN(), not currNumRows
+ if (upperBound) {
+ return Math.round(kll.getN() * (closedBound ?
+ lessThanOrEqualSelectivity(kll, value) : lessThanSelectivity(kll, value)));
+ } else {
+ return Math.round(kll.getN() * (closedBound ?
+ greaterThanOrEqualSelectivity(kll, value) : greaterThanSelectivity(kll, value)));
+ }
+ } catch (RuntimeException e) {
+ LOG.debug("Selectivity computation using histogram failed to parse the boundary value ({}), "
+ + ", using the generic computation strategy", boundValue, e);
Review Comment:
nit: same comment as before about separation between the log message and exception.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/ShowUtils.java:
##########
@@ -209,6 +221,46 @@ public static String convertToString(org.apache.hadoop.hive.metastore.api.Date v
return writableValue.toString();
}
+ private static String convertHistogram(byte[] buffer, ColumnStatisticsData._Fields field) {
Review Comment:
nit: A brief comment for this method would be useful.
##########
ql/src/test/results/clientpositive/beeline/colstats_all_nulls.q.out:
##########
@@ -73,6 +74,7 @@ max_col_len
num_trues
num_falses
bit_vector HL
+histogram
Review Comment:
The fact that the DESCRIBE FORMATTED output is changing by default makes me think whether it is better to not show this field if histogram stats is disabled. Since this is brand new (and experimental) feature, the DESCRIBE output could be made conditional on whether it is enabled. Perhaps this was discussed with other reviewers .. I am not sure.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java:
##########
@@ -1234,17 +1285,70 @@ private long evaluateComparator(Statistics stats, AnnotateStatsProcCtx aspCtx, E
// new estimate for the number of rows
return Math.round(
((maxValue.subtract(value)).divide(maxValue.subtract(minValue), RoundingMode.UP))
- .multiply(BigDecimal.valueOf(numRows))
+ .multiply(BigDecimal.valueOf(currNumRows))
.doubleValue());
}
}
}
} catch (NumberFormatException nfe) {
- return numRows / 3;
+ return currNumRows / 3;
}
}
// default
- return numRows / 3;
+ return currNumRows / 3;
+ }
+
+ private long evaluateComparatorWithHistogram(ColStatistics cs, long currNumRows, String colTypeLowerCase,
+ String boundValue, boolean upperBound, boolean closedBound) {
+ final KllFloatsSketch kll = KllFloatsSketch.heapify(Memory.wrap(cs.getHistogram()));
+
+ if (kll.getN() == 0) {
+ return 0;
+ }
+
+ try {
+ final float value = extractFloatFromLiteralValue(colTypeLowerCase, boundValue);
+
+ // kll ignores null values (i.e., kll.getN() + numNulls = currNumRows), we therefore need to use kll.getN()
+ // instead of currNumRows since the CDF is expressed as a fraction of kll.getN(), not currNumRows
+ if (upperBound) {
+ return Math.round(kll.getN() * (closedBound ?
+ lessThanOrEqualSelectivity(kll, value) : lessThanSelectivity(kll, value)));
+ } else {
+ return Math.round(kll.getN() * (closedBound ?
+ greaterThanOrEqualSelectivity(kll, value) : greaterThanSelectivity(kll, value)));
+ }
+ } catch (RuntimeException e) {
+ LOG.debug("Selectivity computation using histogram failed to parse the boundary value ({}), "
+ + ", using the generic computation strategy", boundValue, e);
+ return currNumRows / 3;
+ }
+ }
+
+ @VisibleForTesting
+ protected static float extractFloatFromLiteralValue(String colTypeLowerCase, String value) {
+ if (colTypeLowerCase.equals(serdeConstants.TINYINT_TYPE_NAME)) {
+ return Byte.parseByte(value);
+ } else if (colTypeLowerCase.equals(serdeConstants.SMALLINT_TYPE_NAME)) {
+ return Short.parseShort(value);
+ } else if (colTypeLowerCase.equals(serdeConstants.INT_TYPE_NAME)) {
+ return Integer.parseInt(value);
+ } else if (colTypeLowerCase.equals(serdeConstants.BIGINT_TYPE_NAME)) {
+ return Long.parseLong(value);
+ } else if (colTypeLowerCase.equals(serdeConstants.FLOAT_TYPE_NAME)) {
+ return Float.parseFloat(value);
+ } else if (colTypeLowerCase.equals(serdeConstants.DOUBLE_TYPE_NAME)) {
+ return (float) Double.parseDouble(value);
+ } else if (colTypeLowerCase.startsWith(serdeConstants.DECIMAL_TYPE_NAME)) {
+ return new BigDecimal(value).floatValue();
Review Comment:
A Decimal value should be represented through a double rather than float since Decimal precision can exceed what float can store.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org