You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/02/02 01:08:36 UTC
[1/2] impala git commit: IMPALA-3562: support column restriction for
compute stats
Repository: impala
Updated Branches:
refs/heads/master d49f629c4 -> 4bd7cc8db
IMPALA-3562: support column restriction for compute stats
The 'compute stats' statement currently computes column-level
statistics for all columns of a table.
This adds potentially unneeded work for columns whose stats
are not needed by queries. It can be especially costly for
very wide tables and unneeded large string fields.
This change modifies the 'compute stats' (non-incremental only)
to support a user-specified list of columns for which stats
should be computed. An example with the extension is as follows:
compute stats my_db.my_table(column_a, column_b);
While the phrase "for columns ..." is commonly used, since
'compute stats' seems fairly unique (vs. 'analyze table ...'),
this change favors brevity with the parenthesized column list.
Whereas currently 'compute stats' is applied to the columns that
can be analyzed, the 'compute stats' in this change results in
an error when a column is specified that cannot be analyzed
(e.g., column does not exist, column is of an unsupported type,
column is a partitioning column). Moreover, an empty column
list can be supplied which means that no columns will be analyzed.
Testing:
- analyzing a subset of columns is already supported (e.g., not all
columns can be analyzed), so the focus with testing is to check
that the user-specified columns are handled as expected.
- tests include: parser tests, ddl analysis, end-to-end tests.
Change-Id: If8b25dd248e578dc7ddd35468125cca12d1b9f27
Reviewed-on: http://gerrit.cloudera.org:8080/9133
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/08ca346f
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/08ca346f
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/08ca346f
Branch: refs/heads/master
Commit: 08ca346f2e917ce2382e8ac532ee016dd5b320fc
Parents: d49f629
Author: Vuk Ercegovac <ve...@cloudera.com>
Authored: Wed Jan 24 12:23:32 2018 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Thu Feb 1 20:27:14 2018 +0000
----------------------------------------------------------------------
fe/src/main/cup/sql-parser.cup | 5 +-
.../impala/analysis/ComputeStatsStmt.java | 65 +++++++--
.../java/org/apache/impala/catalog/Table.java | 2 +-
.../apache/impala/analysis/AnalyzeDDLTest.java | 55 +++++++-
.../org/apache/impala/analysis/ParserTest.java | 37 +++--
.../queries/QueryTest/compute-stats.test | 134 +++++++++++++++++++
.../custom_cluster/test_stats_extrapolation.py | 44 ++++--
7 files changed, 303 insertions(+), 39 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/fe/src/main/cup/sql-parser.cup
----------------------------------------------------------------------
diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup
index 668bb88..dc0199b 100644
--- a/fe/src/main/cup/sql-parser.cup
+++ b/fe/src/main/cup/sql-parser.cup
@@ -1798,7 +1798,10 @@ cascade_val ::=
compute_stats_stmt ::=
KW_COMPUTE KW_STATS table_name:table opt_tablesample:tblsmpl
- {: RESULT = ComputeStatsStmt.createStatsStmt(table, tblsmpl); :}
+ {: RESULT = ComputeStatsStmt.createStatsStmt(table, tblsmpl, null); :}
+ | KW_COMPUTE KW_STATS table_name:table LPAREN opt_ident_list:cols RPAREN
+ opt_tablesample:tblsmpl
+ {: RESULT = ComputeStatsStmt.createStatsStmt(table, tblsmpl, cols); :}
| KW_COMPUTE KW_INCREMENTAL KW_STATS table_name:table
{: RESULT = ComputeStatsStmt.createIncrementalStatsStmt(table, null); :}
| KW_COMPUTE KW_INCREMENTAL KW_STATS table_name:table partition_set:parts
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
index 4e61d86..6ca8dc9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
@@ -22,6 +22,7 @@ import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import org.apache.hadoop.hive.metastore.api.FieldSchema;
import org.apache.impala.authorization.Privilege;
@@ -53,7 +54,7 @@ import com.google.common.collect.Sets;
* clauses used (sampling, partition spec), as well as whether stats extrapolation
* is enabled or not (--enable_stats_extrapolation).
*
- * 1. COMPUTE STATS <table> [TABLESAMPLE SYSTEM(<perc>) [REPEATABLE(<seed>)]]
+ * 1. COMPUTE STATS <table> [(col_list)] [TABLESAMPLE SYSTEM(<perc>) [REPEATABLE(<seed>)]]
* - Stats extrapolation enabled:
* Computes and replaces the table-level row count and total file size, as well as all
* table-level column statistics. Existing partition-objects and their row count are
@@ -71,6 +72,9 @@ import com.google.common.collect.Sets;
* partitions to set the extrapolated numRows statistic. Altering many partitions is
* expensive and so should be avoided in favor of enabling extrapolation.
*
+ * By default, statistics are computed for all columns. To control which columns are
+ * analyzed, a whitelist of columns names can be optionally specified.
+ *
* 2. COMPUTE INCREMENTAL STATS <table> [PARTITION <part_spec>]
* - Stats extrapolation enabled:
* Not supported for now to keep the logic/code simple.
@@ -84,7 +88,7 @@ import com.google.common.collect.Sets;
* If a set of partitions is specified, then the incremental statistics for those
* partitions are recomputed (then merged into the table-level statistics).
*
- * TODO: Allow more coarse/fine grained (db, column)
+ * TODO: Allow more coarse (db)
* TODO: Compute stats on complex types.
*/
public class ComputeStatsStmt extends StatementBase {
@@ -143,6 +147,15 @@ public class ComputeStatsStmt extends StatementBase {
// null if this is a non-incremental computation.
private PartitionSet partitionSet_ = null;
+ // If non-null, represents the user-specified list of columns for computing statistics.
+ // Not supported for incremental statistics.
+ private List<String> columnWhitelist_ = null;
+
+ // The set of columns to be analyzed. Each column is valid: it must exist in the table
+ // schema, it must be of a type that can be analyzed, and cannot refer to a partitioning
+ // column for HDFS tables. If the set is null, no columns are restricted.
+ private Set<Column> validatedColumnWhitelist_ = null;
+
// The maximum number of partitions that may be explicitly selected by filter
// predicates. Any query that selects more than this automatically drops back to a full
// incremental stats recomputation.
@@ -154,15 +167,17 @@ public class ComputeStatsStmt extends StatementBase {
* Should only be constructed via static creation functions.
*/
private ComputeStatsStmt(TableName tableName, TableSampleClause sampleParams,
- boolean isIncremental, PartitionSet partitionSet) {
+ boolean isIncremental, PartitionSet partitionSet, List<String> columns) {
Preconditions.checkState(tableName != null && !tableName.isEmpty());
Preconditions.checkState(isIncremental || partitionSet == null);
Preconditions.checkState(!isIncremental || sampleParams == null);
+ Preconditions.checkState(!isIncremental || columns == null);
tableName_ = tableName;
sampleParams_ = sampleParams;
table_ = null;
isIncremental_ = isIncremental;
partitionSet_ = partitionSet;
+ columnWhitelist_ = columns;
if (partitionSet_ != null) {
partitionSet_.setTableName(tableName);
partitionSet_.setPrivilegeRequirement(Privilege.ALTER);
@@ -174,17 +189,17 @@ public class ComputeStatsStmt extends StatementBase {
* stats should be computed with table sampling.
*/
public static ComputeStatsStmt createStatsStmt(TableName tableName,
- TableSampleClause sampleParams) {
- return new ComputeStatsStmt(tableName, sampleParams, false, null);
+ TableSampleClause sampleParams, List<String> columns) {
+ return new ComputeStatsStmt(tableName, sampleParams, false, null, columns);
}
/**
- * Returns a stmt for COMPUTE INCREMENTAL STATS. The optional 'partitioSet' specifies a
+ * Returns a stmt for COMPUTE INCREMENTAL STATS. The optional 'partitionSet' specifies a
* set of partitions whose stats should be computed.
*/
public static ComputeStatsStmt createIncrementalStatsStmt(TableName tableName,
PartitionSet partitionSet) {
- return new ComputeStatsStmt(tableName, null, true, partitionSet);
+ return new ComputeStatsStmt(tableName, null, true, partitionSet, null);
}
private List<String> getBaseColumnStatsQuerySelectList(Analyzer analyzer) {
@@ -196,6 +211,9 @@ public class ComputeStatsStmt extends StatementBase {
for (int i = startColIdx; i < table_.getColumns().size(); ++i) {
Column c = table_.getColumns().get(i);
+ if (validatedColumnWhitelist_ != null && !validatedColumnWhitelist_.contains(c)) {
+ continue;
+ }
if (ignoreColumn(c)) continue;
// NDV approximation function. Add explicit alias for later identification when
@@ -324,6 +342,26 @@ public class ComputeStatsStmt extends StatementBase {
isIncremental_ = false;
}
+ if (columnWhitelist_ != null) {
+ validatedColumnWhitelist_ = Sets.newHashSet();
+ for (String colName : columnWhitelist_) {
+ Column col = table_.getColumn(colName);
+ if (col == null) {
+ throw new AnalysisException(colName + " not found in table: " +
+ table_.getName());
+ }
+ if (table_ instanceof HdfsTable && table_.isClusteringColumn(col)) {
+ throw new AnalysisException("COMPUTE STATS not supported for partitioning " +
+ "column " + col.getName() + " of HDFS table.");
+ }
+ if (ignoreColumn(col)) {
+ throw new AnalysisException("COMPUTE STATS not supported for column " +
+ col.getName() + " of complex type:" + col.getType().toSql());
+ }
+ validatedColumnWhitelist_.add(col);
+ }
+ }
+
HdfsTable hdfsTable = null;
if (table_ instanceof HdfsTable) {
hdfsTable = (HdfsTable)table_;
@@ -683,8 +721,13 @@ public class ComputeStatsStmt extends StatementBase {
}
public double getEffectiveSamplingPerc() { return effectiveSamplePerc_; }
+
+ /**
+ * For testing.
+ */
public String getTblStatsQuery() { return tableStatsQueryStr_; }
public String getColStatsQuery() { return columnStatsQueryStr_; }
+ public Set<Column> getValidatedColumnWhitelist() { return validatedColumnWhitelist_; }
/**
* Returns true if this statement computes stats on Parquet partitions only,
@@ -707,9 +750,15 @@ public class ComputeStatsStmt extends StatementBase {
@Override
public String toSql() {
if (!isIncremental_) {
+ StringBuilder columnList = new StringBuilder();
+ if (columnWhitelist_ != null) {
+ columnList.append("(");
+ columnList.append(Joiner.on(", ").join(columnWhitelist_));
+ columnList.append(")");
+ }
String tblsmpl = "";
if (sampleParams_ != null) tblsmpl = " " + sampleParams_.toSql();
- return "COMPUTE STATS " + tableName_.toSql() + tblsmpl;
+ return "COMPUTE STATS " + tableName_.toSql() + columnList.toString() + tblsmpl;
} else {
return "COMPUTE INCREMENTAL STATS " + tableName_.toSql() +
partitionSet_ == null ? "" : partitionSet_.toSql();
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/fe/src/main/java/org/apache/impala/catalog/Table.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/Table.java b/fe/src/main/java/org/apache/impala/catalog/Table.java
index a6536ba..aca9409 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Table.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Table.java
@@ -508,7 +508,7 @@ public abstract class Table extends CatalogObjectImpl {
}
/**
- * Case-insensitive lookup.
+ * Case-insensitive lookup. Returns null if the column with 'name' is not found.
*/
public Column getColumn(String name) { return colsByName_.get(name.toLowerCase()); }
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
index 122b49d..4124493 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -17,12 +17,14 @@
package org.apache.impala.analysis;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Set;
import java.util.UUID;
import org.apache.commons.lang3.StringUtils;
@@ -34,6 +36,7 @@ import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.impala.catalog.ArrayType;
import org.apache.impala.catalog.Catalog;
import org.apache.impala.catalog.CatalogException;
+import org.apache.impala.catalog.Column;
import org.apache.impala.catalog.ColumnStats;
import org.apache.impala.catalog.DataSource;
import org.apache.impala.catalog.DataSourceTable;
@@ -60,6 +63,7 @@ import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
public class AnalyzeDDLTest extends FrontendTestBase {
@@ -1180,16 +1184,37 @@ public class AnalyzeDDLTest extends FrontendTestBase {
return checkComputeStatsStmt(stmt, analyzer, null);
}
+ /**
+ * Analyzes 'stmt' and checks that the table-level and column-level SQL that is used
+ * to compute the stats is valid. Returns the analyzed statement.
+ */
ComputeStatsStmt checkComputeStatsStmt(String stmt, Analyzer analyzer,
String expectedWarning) throws AnalysisException {
ParseNode parseNode = AnalyzesOk(stmt, analyzer, expectedWarning);
assertTrue(parseNode instanceof ComputeStatsStmt);
ComputeStatsStmt parsedStmt = (ComputeStatsStmt)parseNode;
AnalyzesOk(parsedStmt.getTblStatsQuery());
- AnalyzesOk(parsedStmt.getColStatsQuery());
+ String colsQuery = parsedStmt.getColStatsQuery();
+ if (colsQuery != null) AnalyzesOk(colsQuery);
return parsedStmt;
}
+ /**
+ * In addition to the validation for checkComputeStatsStmt(String), checks that the
+ * whitelisted columns match 'expColNames'.
+ */
+ void checkComputeStatsStmt(String stmt, List<String> expColNames)
+ throws AnalysisException {
+ ComputeStatsStmt parsedStmt = checkComputeStatsStmt(stmt);
+ Set<Column> actCols = parsedStmt.getValidatedColumnWhitelist();
+ if (expColNames == null) assertTrue("Expected no whitelist.", actCols == null);
+ assertTrue("Expected whitelist.", actCols != null);
+ Set<String> actColSet = Sets.newHashSet();
+ for (Column col: actCols) actColSet.add(col.getName());
+ Set<String> expColSet = Sets.newHashSet(expColNames);
+ assertEquals(actColSet, expColSet);
+ }
+
@Test
public void TestComputeStats() throws AnalysisException {
// Analyze the stmt itself as well as the generated child queries.
@@ -1197,6 +1222,28 @@ public class AnalyzeDDLTest extends FrontendTestBase {
checkComputeStatsStmt("compute stats functional_hbase.alltypes");
// Test that complex-typed columns are ignored.
checkComputeStatsStmt("compute stats functional.allcomplextypes");
+ // Test legal column restriction.
+ checkComputeStatsStmt("compute stats functional.alltypes (int_col, double_col)",
+ Lists.newArrayList("int_col", "double_col"));
+ // Test legal column restriction with duplicate columns specified.
+ checkComputeStatsStmt(
+ "compute stats functional.alltypes (int_col, double_col, int_col)",
+ Lists.newArrayList("int_col", "double_col"));
+ // Test empty column restriction.
+ checkComputeStatsStmt("compute stats functional.alltypes ()",
+ new ArrayList<String>());
+ // Test column restriction of a column that does not exist.
+ AnalysisError("compute stats functional.alltypes(int_col, bogus_col, double_col)",
+ "bogus_col not found in table:");
+ // Test column restriction of a column with an unsupported type.
+ AnalysisError("compute stats functional.allcomplextypes(id, map_map_col)",
+ "COMPUTE STATS not supported for column");
+ // Test column restriction of an Hdfs table partitioning column.
+ AnalysisError("compute stats functional.stringpartitionkey(string_col)",
+ "COMPUTE STATS not supported for partitioning");
+ // Test column restriction of an HBase key column.
+ checkComputeStatsStmt("compute stats functional_hbase.testtbl(id)",
+ Lists.newArrayList("id"));
// Cannot compute stats on a database.
AnalysisError("compute stats tbl_does_not_exist",
@@ -1283,7 +1330,11 @@ public class AnalyzeDDLTest extends FrontendTestBase {
// changes. Expect a sample between 4 and 6 of the 24 total files.
Assert.assertTrue(adjustedStmt.getEffectiveSamplingPerc() >= 4.0 / 24);
Assert.assertTrue(adjustedStmt.getEffectiveSamplingPerc() <= 6.0 / 24);
-
+ // Checks that whitelisted columns works with tablesample.
+ checkComputeStatsStmt(
+ "compute stats functional.alltypes (int_col, double_col) tablesample " +
+ "system (55) repeatable(1)",
+ Lists.newArrayList("int_col", "double_col"));
AnalysisError("compute stats functional.alltypes tablesample system (101)",
"Invalid percent of bytes value '101'. " +
"The percent of bytes to sample must be between 0 and 100.");
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
index 40585bf..8dd4898 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
@@ -3165,21 +3165,32 @@ public class ParserTest extends FrontendTestBase {
@Test
public void TestComputeDropStats() {
String[] prefixes = {"compute", "drop"};
+ String[] okSuffixes = {"stats bar", "stats `bar`", "stats foo.bar",
+ "stats `foo`.`bar`"};
+ String[] okComputeSuffixes = {"(ab)", "(ab, bc)", "()"};
+ String[] errorSuffixes = {
+ // Missing table name.
+ "stats",
+ // Missing 'stats' keyword.
+ "`bar`",
+ // Cannot use string literal as table name.
+ "stats 'foo'",
+ // Cannot analyze multiple tables in one stmt.
+ "stats foo bar"
+ };
for (String prefix: prefixes) {
- ParsesOk(prefix + " stats bar");
- ParsesOk(prefix + " stats `bar`");
- ParsesOk(prefix + " stats foo.bar");
- ParsesOk(prefix + " stats `foo`.`bar`");
-
- // Missing table name.
- ParserError(prefix + " stats");
- // Missing 'stats' keyword.
- ParserError(prefix + " foo");
- // Cannot use string literal as table name.
- ParserError(prefix + " stats 'foo'");
- // Cannot analyze multiple tables in one stmt.
- ParserError(prefix + " stats foo bar");
+ for (String suffix: okSuffixes) {
+ ParsesOk(prefix + " " + suffix);
+ }
+ for (String suffix: errorSuffixes) {
+ ParserError(prefix + " " + suffix);
+ }
+ }
+ for (String suffix: okSuffixes) {
+ for (String computeSuffix: okComputeSuffixes) {
+ ParsesOk("compute" + " " + suffix + " " + computeSuffix);
+ }
}
}
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test b/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
index 92aa0db..b7494f0 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
@@ -208,6 +208,140 @@ COLUMN, TYPE, #DISTINCT VALUES, #NULLS, MAX SIZE, AVG SIZE
STRING, STRING, BIGINT, BIGINT, BIGINT, DOUBLE
====
---- QUERY
+# Restricts stats to a subset of columns.
+create table alltypes_for_coltest like functional.alltypes;
+insert into alltypes_for_coltest partition(year, month)
+select * from functional.alltypes;
+====
+---- QUERY
+compute stats alltypes_for_coltest(tinyint_col, float_col)
+---- RESULTS
+'Updated 24 partition(s) and 2 column(s).'
+---- TYPES
+STRING
+====
+---- QUERY
+show table stats alltypes_for_coltest
+---- LABELS
+YEAR, MONTH, #ROWS, #FILES, SIZE, BYTES CACHED, CACHE REPLICATION, FORMAT, INCREMENTAL STATS, LOCATION
+---- RESULTS
+'2009','1',310,1,'24.56KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','2',280,1,'22.27KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','3',310,1,'24.67KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','4',300,1,'24.06KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','5',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','6',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','7',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','8',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','9',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','10',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','11',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','12',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','1',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','2',280,1,'22.54KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','3',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','4',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','5',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','6',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','7',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','8',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','9',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','10',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','11',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','12',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'Total','',7300,24,'586.84KB','0B','','','',''
+---- TYPES
+STRING, STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING
+====
+---- QUERY
+show column stats alltypes_for_coltest
+---- LABELS
+COLUMN, TYPE, #DISTINCT VALUES, #NULLS, MAX SIZE, AVG SIZE
+---- RESULTS
+'id','INT',-1,-1,4,4
+'bool_col','BOOLEAN',-1,-1,1,1
+'tinyint_col','TINYINT',10,-1,1,1
+'smallint_col','SMALLINT',-1,-1,2,2
+'int_col','INT',-1,-1,4,4
+'bigint_col','BIGINT',-1,-1,8,8
+'float_col','FLOAT',10,-1,4,4
+'double_col','DOUBLE',-1,-1,8,8
+'date_string_col','STRING',-1,-1,-1,-1
+'string_col','STRING',-1,-1,-1,-1
+'timestamp_col','TIMESTAMP',-1,-1,16,16
+'year','INT',2,0,4,4
+'month','INT',12,0,4,4
+---- TYPES
+STRING, STRING, BIGINT, BIGINT, BIGINT, DOUBLE
+====
+---- QUERY
+# Computes only table statistics; no column statistics.
+create table alltypes_no_col_stats like functional.alltypes;
+insert into alltypes_no_col_stats partition(year, month)
+select * from functional.alltypes;
+====
+---- QUERY
+compute stats alltypes_no_col_stats()
+---- RESULTS
+'Updated 24 partition(s) and 0 column(s).'
+---- TYPES
+STRING
+====
+---- QUERY
+show table stats alltypes_no_col_stats
+---- LABELS
+YEAR, MONTH, #ROWS, #FILES, SIZE, BYTES CACHED, CACHE REPLICATION, FORMAT, INCREMENTAL STATS, LOCATION
+---- RESULTS
+'2009','1',310,1,'24.56KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','2',280,1,'22.27KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','3',310,1,'24.67KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','4',300,1,'24.06KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','5',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','6',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','7',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','8',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','9',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','10',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','11',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2009','12',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','1',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','2',280,1,'22.54KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','3',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','4',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','5',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','6',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','7',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','8',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','9',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','10',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','11',300,1,'24.16KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'2010','12',310,1,'24.97KB','NOT CACHED','NOT CACHED','TEXT','false',regex:.*
+'Total','',7300,24,'586.84KB','0B','','','',''
+---- TYPES
+STRING, STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING
+====
+---- QUERY
+show column stats alltypes_no_col_stats
+---- LABELS
+COLUMN, TYPE, #DISTINCT VALUES, #NULLS, MAX SIZE, AVG SIZE
+---- RESULTS
+'id','INT',-1,-1,4,4
+'bool_col','BOOLEAN',-1,-1,1,1
+'tinyint_col','TINYINT',-1,-1,1,1
+'smallint_col','SMALLINT',-1,-1,2,2
+'int_col','INT',-1,-1,4,4
+'bigint_col','BIGINT',-1,-1,8,8
+'float_col','FLOAT',-1,-1,4,4
+'double_col','DOUBLE',-1,-1,8,8
+'date_string_col','STRING',-1,-1,-1,-1
+'string_col','STRING',-1,-1,-1,-1
+'timestamp_col','TIMESTAMP',-1,-1,16,16
+'year','INT',2,0,4,4
+'month','INT',12,0,4,4
+---- TYPES
+STRING, STRING, BIGINT, BIGINT, BIGINT, DOUBLE
+====
+---- QUERY
# Add partitions with NULL values and check for stats.
alter table alltypes add partition (year=NULL, month=NULL)
---- RESULTS
http://git-wip-us.apache.org/repos/asf/impala/blob/08ca346f/tests/custom_cluster/test_stats_extrapolation.py
----------------------------------------------------------------------
diff --git a/tests/custom_cluster/test_stats_extrapolation.py b/tests/custom_cluster/test_stats_extrapolation.py
index ef0b675..65aa9f1 100644
--- a/tests/custom_cluster/test_stats_extrapolation.py
+++ b/tests/custom_cluster/test_stats_extrapolation.py
@@ -57,10 +57,10 @@ class TestStatsExtrapolation(CustomClusterTestSuite):
# Test partitioned table.
part_test_tbl = unique_database + ".alltypes"
self.clone_table("functional.alltypes", part_test_tbl, True, vector)
- self.__run_sampling_test(part_test_tbl, "functional.alltypes", 1, 3)
- self.__run_sampling_test(part_test_tbl, "functional.alltypes", 10, 7)
- self.__run_sampling_test(part_test_tbl, "functional.alltypes", 20, 13)
- self.__run_sampling_test(part_test_tbl, "functional.alltypes", 100, 99)
+ self.__run_sampling_test(part_test_tbl, "", "functional.alltypes", 1, 3)
+ self.__run_sampling_test(part_test_tbl, "", "functional.alltypes", 10, 7)
+ self.__run_sampling_test(part_test_tbl, "", "functional.alltypes", 20, 13)
+ self.__run_sampling_test(part_test_tbl, "", "functional.alltypes", 100, 99)
# Test unpartitioned table.
nopart_test_tbl = unique_database + ".alltypesnopart"
@@ -70,15 +70,15 @@ class TestStatsExtrapolation(CustomClusterTestSuite):
nopart_test_tbl_exp = unique_database + ".alltypesnopart_exp"
self.clone_table(nopart_test_tbl, nopart_test_tbl_exp, False, vector)
self.client.execute("compute stats {0}".format(nopart_test_tbl_exp))
- self.__run_sampling_test(nopart_test_tbl, nopart_test_tbl_exp, 1, 3)
- self.__run_sampling_test(nopart_test_tbl, nopart_test_tbl_exp, 10, 7)
- self.__run_sampling_test(nopart_test_tbl, nopart_test_tbl_exp, 20, 13)
- self.__run_sampling_test(nopart_test_tbl, nopart_test_tbl_exp, 100, 99)
+ self.__run_sampling_test(nopart_test_tbl, "", nopart_test_tbl_exp, 1, 3)
+ self.__run_sampling_test(nopart_test_tbl, "", nopart_test_tbl_exp, 10, 7)
+ self.__run_sampling_test(nopart_test_tbl, "", nopart_test_tbl_exp, 20, 13)
+ self.__run_sampling_test(nopart_test_tbl, "", nopart_test_tbl_exp, 100, 99)
# Test empty table.
empty_test_tbl = unique_database + ".empty"
self.clone_table("functional.alltypes", empty_test_tbl, False, vector)
- self.__run_sampling_test(empty_test_tbl, empty_test_tbl, 10, 7)
+ self.__run_sampling_test(empty_test_tbl, "", empty_test_tbl, 10, 7)
# Test wide table. Should not crash or error. This takes a few minutes so restrict
# to exhaustive.
@@ -88,13 +88,29 @@ class TestStatsExtrapolation(CustomClusterTestSuite):
self.client.execute(
"compute stats {0} tablesample system(10)".format(wide_test_tbl))
- def __run_sampling_test(self, tbl, expected_tbl, perc, seed):
+ # Test column subset.
+ column_subset_tbl = unique_database + ".column_subset"
+ columns = "(int_col, string_col)"
+ self.clone_table("functional.alltypes", column_subset_tbl, True, vector)
+ self.__run_sampling_test(column_subset_tbl, columns, "functional.alltypes", 1, 3)
+ self.__run_sampling_test(column_subset_tbl, columns, "functional.alltypes", 10, 7)
+ self.__run_sampling_test(column_subset_tbl, columns, "functional.alltypes", 20, 13)
+ self.__run_sampling_test(column_subset_tbl, columns, "functional.alltypes", 100, 99)
+
+ # Test no columns.
+ no_column_tbl = unique_database + ".no_columns"
+ columns = "()"
+ self.clone_table("functional.alltypes", no_column_tbl, True, vector)
+ self.__run_sampling_test(no_column_tbl, columns, "functional.alltypes", 10, 7)
+
+ def __run_sampling_test(self, tbl, cols, expected_tbl, perc, seed):
"""Drops stats on 'tbl' and then runs COMPUTE STATS TABLESAMPLE on 'tbl' with the
- given sampling percent and random seed. Checks that the resulting table and column
- stats are reasoanbly close to those of 'expected_tbl'."""
+ given column restriction clause, sampling percent and random seed. Checks that
+ the resulting table and column stats are reasoanbly close to those of
+ 'expected_tbl'."""
self.client.execute("drop stats {0}".format(tbl))
- self.client.execute("compute stats {0} tablesample system ({1}) repeatable ({2})"\
- .format(tbl, perc, seed))
+ self.client.execute("compute stats {0}{1} tablesample system ({2}) repeatable ({3})"\
+ .format(tbl, cols, perc, seed))
self.__check_table_stats(tbl, expected_tbl)
self.__check_column_stats(tbl, expected_tbl)
[2/2] impala git commit: IMPALA-6429: Fix decimal division
Posted by ta...@apache.org.
IMPALA-6429: Fix decimal division
Before this patch, it was possible for an overflow to not be detected
when doing a decimal division. When scaling up the dividend before
doing the division, we do not check for overflow. This is ok if the
we are scaling up by 10^38 or less because the result is guaranteed to
fit into 256 bits. However, when we are scaling up by more than 38, the
result may not fit into 256 bits and overflow. This overflow may not be
detected.
The problem is fixed by checking for overflow when scaling up the
dividend. The overflow check is done efficiently, by counting the
leading zeros. I added a test to prove that this check is correct.
Testing:
- Added some BE tests
- I ran a few benchmarks and did not see any performance regressions
Change-Id: Ibd1075d9c78986cd975dd29c1125d71ba6560c23
Reviewed-on: http://gerrit.cloudera.org:8080/9114
Reviewed-by: Taras Bobrovytsky <tb...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/4bd7cc8d
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/4bd7cc8d
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/4bd7cc8d
Branch: refs/heads/master
Commit: 4bd7cc8dbf2f07db3468e1feb595cd16a7cd81e3
Parents: 08ca346
Author: Taras Bobrovytsky <tb...@cloudera.com>
Authored: Tue Jan 23 15:19:59 2018 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Feb 2 01:01:36 2018 +0000
----------------------------------------------------------------------
be/src/exprs/expr-test.cc | 62 +++++++++++++++++++++
be/src/runtime/decimal-value.inline.h | 87 ++++++++++++++++++------------
be/src/util/decimal-util.h | 18 ++++---
3 files changed, 127 insertions(+), 40 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/4bd7cc8d/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 21911a1..a78fcae 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -2419,6 +2419,25 @@ DecimalTestCase decimal_cases[] = {
{ "18014118346046923173168730371588410/-340282366920938463463374607431768200",
{{ false, false, 0, 38, 0 },
{ false, false, -52939, 38, 6 }}},
+ // IMPALA-6429: Test overflow detection when scaling up the dividend by more than 38.
+ // The bug can be trigerred only by these specific values. These values were generated
+ // by the fuzz test.
+ { "cast(70685438201098443655665080810945040.194 as decimal(38,3)) / "
+ "cast(0.85070591730234615865843651857942052863 as decimal(38,38))",
+ {{ false, true, 0, 38, 38 },
+ { true, false, 0, 38, 6 }}},
+ { "cast(9269574547799442144750864826042582 as decimal(38,2)) / "
+ "cast(0.2475880078570760549798248447 as decimal(38,38))",
+ {{ false, true, 0, 38, 38 },
+ { true, false, 0, 38, 6 }}},
+ { "cast(19770219132749848273961352693131418.9 as decimal(36,1)) / "
+ "cast(0.973940341920032002 as decimal(38,38))",
+ {{ false, true, 0, 38, 38 },
+ { true, false, 0, 38, 6 }}},
+ { "cast(100000000000000000000000000000000 as decimal(36,3)) / "
+ "cast(1 as decimal(1,0))",
+ {{ false, false, StringToInt128("10000000000000000000000000000000000000"), 38, 5 },
+ { true, false, 0, 38, 6 }}},
// Test modulo operator
{ "cast(1.23 as decimal(8,2)) % cast(1 as decimal(10,3))",
{{ false, false, 230, 9, 3 }}},
@@ -2821,6 +2840,48 @@ DecimalTestCase decimal_cases[] = {
{ true, false, 0, 38, 0 }}},
};
+void TestScaleBy() {
+ // IMPALA-6429: There is a shortcut in the decimal division. If we estimate that the
+ // dividend requires more than 255 bits after scaling up, we overflow right away. This
+ // test proves that it is correct to do this and no other checks are needed.
+ for (int scale_by = 0; scale_by < 38 * 2 + 1; ++scale_by) {
+ for (int num_bits = 1; num_bits < 128; ++num_bits) {
+ // We set the dividend to be the smallest number that requires a certain number of
+ // bits.
+ int128_t dividend = 1;
+ dividend <<= num_bits - 1;
+ int256_t scaled_up_dividend = DecimalUtil::MultiplyByScale<int256_t>(
+ ConvertToInt256(dividend), scale_by, true);
+ int256_t scale_multiplier = DecimalUtil::GetScaleMultiplier<int256_t>(scale_by);
+ if (detail::MaxBitsRequiredAfterScaling(dividend, scale_by) <= 255) {
+ // If we estimate that the scaled up dividend requires 255 bits or less, verify
+ // that we do not overflow when scaling up.
+ EXPECT_TRUE(scaled_up_dividend / scale_multiplier == ConvertToInt256(dividend));
+ EXPECT_TRUE(
+ (-scaled_up_dividend) / scale_multiplier == ConvertToInt256(-dividend));
+ } else {
+ // If we estimate that scaled up dividend requres more than 255 bits, we want to
+ // verify that it is safe to set the result of the division to overflow.
+ if (scaled_up_dividend / scale_multiplier == ConvertToInt256(dividend)) {
+ // In this case, scaling up did not overflow. Verify that the scaled up
+ // dividend is too large. Even if we divide it by the largest possible divisor
+ // the result is larger than MAX_UNSCALED_DECIMAL16, which means the division
+ // overflows in all cases.
+ EXPECT_TRUE((-scaled_up_dividend) / scale_multiplier ==
+ ConvertToInt256(-dividend));
+ int256_t max_divisor = ConvertToInt256(DecimalUtil::MAX_UNSCALED_DECIMAL16);
+ EXPECT_TRUE(scaled_up_dividend / max_divisor > max_divisor);
+ EXPECT_TRUE((-scaled_up_dividend) / max_divisor < -max_divisor);
+ } else {
+ // There was an overflow when scaling up.
+ EXPECT_TRUE((-scaled_up_dividend) / scale_multiplier !=
+ ConvertToInt256(-dividend));
+ }
+ }
+ }
+ }
+}
+
TEST_F(ExprTest, DecimalArithmeticExprs) {
// Test with both decimal_v2={false, true}
for (int v2: { 0, 1 }) {
@@ -2853,6 +2914,7 @@ TEST_F(ExprTest, DecimalArithmeticExprs) {
}
executor_->PopExecOption();
}
+ TestScaleBy();
}
// Tests for expressions that mix decimal and non-decimal arguments with DECIMAL_V2=false.
http://git-wip-us.apache.org/repos/asf/impala/blob/4bd7cc8d/be/src/runtime/decimal-value.inline.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/decimal-value.inline.h b/be/src/runtime/decimal-value.inline.h
index abcdad6..8b5f158 100644
--- a/be/src/runtime/decimal-value.inline.h
+++ b/be/src/runtime/decimal-value.inline.h
@@ -67,7 +67,7 @@ inline DecimalValue<T> DecimalValue<T>::FromInt(int precision, int scale, int64_
*overflow = true;
return DecimalValue();
}
- return DecimalValue(DecimalUtil::MultiplyByScale<T>(d, scale));
+ return DecimalValue(DecimalUtil::MultiplyByScale<T>(d, scale, false));
}
template<typename T>
@@ -140,29 +140,45 @@ inline DecimalValue<T> DecimalValue<T>::ScaleTo(int src_scale, int dst_scale,
namespace detail {
-// Helper function that checks for multiplication overflow. We only check for overflow
-// if may_overflow is false.
-template <typename T>
-inline T SafeMultiply(T a, T b, bool may_overflow) {
- T result = a * b;
- DCHECK(may_overflow || a == 0 || result / a == b);
+// Suppose we have a number that requires x bits to be represented and we scale it up by
+// 10^scale_by. Let's say now y bits are required to represent it. This function returns
+// the maximum possible y - x for a given 'scale_by'.
+inline int MaxBitsRequiredIncreaseAfterScaling(int scale_by) {
+ // We rely on the following formula:
+ // bits_required(x * 10^y) <= bits_required(x) + floor(log2(10^y)) + 1
+ // We precompute floor(log2(10^x)) + 1 for x = 0, 1, 2...75, 76
+ DCHECK_GE(scale_by, 0);
+ DCHECK_LE(scale_by, 76);
+ static const int floor_log2_plus_one[] = {
+ 0, 4, 7, 10, 14, 17, 20, 24, 27, 30,
+ 34, 37, 40, 44, 47, 50, 54, 57, 60, 64,
+ 67, 70, 74, 77, 80, 84, 87, 90, 94, 97,
+ 100, 103, 107, 110, 113, 117, 120, 123, 127, 130,
+ 133, 137, 140, 143, 147, 150, 153, 157, 160, 163,
+ 167, 170, 173, 177, 180, 183, 187, 190, 193, 196,
+ 200, 203, 206, 210, 213, 216, 220, 223, 226, 230,
+ 233, 236, 240, 243, 246, 250, 253 };
+ return floor_log2_plus_one[scale_by];
+}
+
+// If we have a number with 'num_lz' leading zeros, and we scale it up by 10^scale_by,
+// this function returns the minimum number of leading zeros the result can have.
+inline int MinLeadingZerosAfterScaling(int num_lz, int scale_by) {
+ DCHECK_GE(scale_by, 0);
+ DCHECK_LE(scale_by, 76);
+ int result = num_lz - MaxBitsRequiredIncreaseAfterScaling(scale_by);
return result;
}
-// If we have a number with 'num_lz' leading zeros, and we scale it up by 10^scale_diff,
-// this function returns the minimum number of leading zeros the result would have.
-inline int MinLeadingZerosAfterScaling(int num_lz, int scale_diff) {
- DCHECK_GE(scale_diff, 0);
- DCHECK_LE(scale_diff, 38);
- // We will rely on the following formula to estimate the number of leading zeros after
- // scaling up: Lz(a*b) >= Lz(a) - floor(log2(b)) - 1
- // We precompute floor(log2(10^b)) for b = 0, 1, 2, 3...
- static const int floor_log2[] = {
- 0, 3, 6, 9, 13, 16, 19, 23, 26, 29,
- 33, 36, 39, 43, 46, 49, 53, 56, 59, 63,
- 66, 69, 73, 76, 79, 83, 86, 89, 93, 96,
- 99, 102, 106, 109, 112, 116, 119, 122, 126 };
- return num_lz - floor_log2[scale_diff] - 1;
+// Returns the maximum possible number of bits required to represent num * 10^scale_by.
+inline int MaxBitsRequiredAfterScaling(int128_t num, int scale_by) {
+ // TODO: We are doing a lot of these abs() operations on int128_t in many places in our
+ // decimal math code. It might make sense to do this upfront, then do the calculations
+ // in unsigned math and adjust the sign at the end.
+ int num_occupied = 128 - BitUtil::CountLeadingZeros<int128_t>(abs(num));
+ DCHECK_GE(scale_by, 0);
+ DCHECK_LE(scale_by, 76);
+ return num_occupied + MaxBitsRequiredIncreaseAfterScaling(scale_by);
}
// Returns the minimum number of leading zero x or y would have after one of them gets
@@ -241,7 +257,7 @@ inline int128_t AddLarge(int128_t x, int x_scale, int128_t y, int y_scale,
left > (DecimalUtil::MAX_UNSCALED_DECIMAL16 - right) / mult)) {
*overflow = true;
}
- return SafeMultiply(left, mult, *overflow) + right;
+ return DecimalUtil::SafeMultiply(left, mult, *overflow) + right;
}
// Subtracts numbers that are large enough so that we can't subtract directly. Neither
@@ -294,7 +310,7 @@ inline int128_t SubtractLarge(int128_t x, int x_scale, int128_t y, int y_scale,
if (UNLIKELY(abs(left) > (DecimalUtil::MAX_UNSCALED_DECIMAL16 - abs(right)) / mult)) {
*overflow = true;
}
- return SafeMultiply(left, mult, *overflow) + right;
+ return DecimalUtil::SafeMultiply(left, mult, *overflow) + right;
}
}
@@ -412,7 +428,7 @@ DecimalValue<RESULT_T> DecimalValue<T>::Multiply(int this_scale,
}
} else {
if (delta_scale == 0) {
- result = detail::SafeMultiply(x, y, false);
+ result = DecimalUtil::SafeMultiply(x, y, false);
if (UNLIKELY(result_precision == ColumnType::MAX_PRECISION &&
abs(result) > DecimalUtil::MAX_UNSCALED_DECIMAL16)) {
// An overflow is possible here, if, for example, x = (2^64 - 1) and
@@ -420,7 +436,7 @@ DecimalValue<RESULT_T> DecimalValue<T>::Multiply(int this_scale,
*overflow = true;
}
} else if (LIKELY(delta_scale <= 38)) {
- result = detail::SafeMultiply(x, y, false);
+ result = DecimalUtil::SafeMultiply(x, y, false);
// The largest value that result can have here is (2^64 - 1) * (2^63 - 1), which is
// greater than MAX_UNSCALED_DECIMAL16.
result = DecimalUtil::ScaleDownAndRound<RESULT_T>(result, delta_scale, round);
@@ -451,7 +467,7 @@ inline DecimalValue<RESULT_T> DecimalValue<T>::Divide(int this_scale,
const DecimalValue& other, int other_scale, int result_precision, int result_scale,
bool round, bool* is_nan, bool* overflow) const {
DCHECK_GE(result_scale + other_scale, this_scale);
- if (other.value() == 0) {
+ if (UNLIKELY(other.value() == 0)) {
// Divide by 0.
*is_nan = true;
return DecimalValue<RESULT_T>();
@@ -459,11 +475,17 @@ inline DecimalValue<RESULT_T> DecimalValue<T>::Divide(int this_scale,
// We need to scale x up by the result scale and then do an integer divide.
// This truncates the result to the output scale.
int scale_by = result_scale + other_scale - this_scale;
+ DCHECK_GE(scale_by, 0);
// Use higher precision ints for intermediates to avoid overflows. Divides lead to
// large numbers very quickly (and get eliminated by the int divide).
if (sizeof(T) == 16) {
int128_t x_sp = value();
- int256_t x = DecimalUtil::MultiplyByScale<int256_t>(ConvertToInt256(x_sp), scale_by);
+ // There is a test in expr-test.cc that shows that it OK to check for overflow this
+ // way (and that no additional checks are required).
+ bool ovf = scale_by > 38 && detail::MaxBitsRequiredAfterScaling(x_sp, scale_by) > 255;
+ int256_t x = DecimalUtil::MultiplyByScale<int256_t>(
+ ConvertToInt256(x_sp), scale_by, ovf);
+ *overflow |= ovf;
int128_t y_sp = other.value();
int256_t y = ConvertToInt256(y_sp);
int128_t r = ConvertToInt128(x / y, DecimalUtil::MAX_UNSCALED_DECIMAL16, overflow);
@@ -487,8 +509,7 @@ inline DecimalValue<RESULT_T> DecimalValue<T>::Divide(int this_scale,
}
return DecimalValue<RESULT_T>(r);
} else {
- DCHECK(DecimalUtil::GetScaleMultiplier<RESULT_T>(scale_by) > 0);
- int128_t x = DecimalUtil::MultiplyByScale<RESULT_T>(value(), scale_by);
+ int128_t x = DecimalUtil::MultiplyByScale<RESULT_T>(value(), scale_by, false);
int128_t y = other.value();
int128_t r = x / y;
if (round) {
@@ -592,9 +613,9 @@ inline void DecimalValue<T>::AdjustToSameScale(const DecimalValue<T>& x, int x_s
*y_scaled = y.value();
} else if (delta_scale > 0) {
*x_scaled = x.value();
- *y_scaled = detail::SafeMultiply<RESULT_T>(y.value(), scale_factor, false);
+ *y_scaled = DecimalUtil::SafeMultiply<RESULT_T>(y.value(), scale_factor, false);
} else {
- *x_scaled = detail::SafeMultiply<RESULT_T>(x.value(), scale_factor, false);
+ *x_scaled = DecimalUtil::SafeMultiply<RESULT_T>(x.value(), scale_factor, false);
*y_scaled = y.value();
}
}
@@ -629,9 +650,9 @@ inline int Decimal16Value::Compare(int this_scale, const Decimal16Value& other,
int256_t y = ConvertToInt256(other.value());
int delta_scale = this_scale - other_scale;
if (delta_scale > 0) {
- y = DecimalUtil::MultiplyByScale<int256_t>(y, delta_scale);
+ y = DecimalUtil::MultiplyByScale<int256_t>(y, delta_scale, false);
} else if (delta_scale < 0) {
- x = DecimalUtil::MultiplyByScale<int256_t>(x, -delta_scale);
+ x = DecimalUtil::MultiplyByScale<int256_t>(x, -delta_scale, false);
}
if (x == y) return 0;
if (x < y) return -1;
http://git-wip-us.apache.org/repos/asf/impala/blob/4bd7cc8d/be/src/util/decimal-util.h
----------------------------------------------------------------------
diff --git a/be/src/util/decimal-util.h b/be/src/util/decimal-util.h
index 6686a20..53cedb1 100644
--- a/be/src/util/decimal-util.h
+++ b/be/src/util/decimal-util.h
@@ -42,16 +42,20 @@ class DecimalUtil {
(MAX_UNSCALED_DECIMAL8 + (1 + MAX_UNSCALED_DECIMAL8) *
static_cast<int128_t>(MAX_UNSCALED_DECIMAL8));
- /// TODO: do we need to handle overflow here or at a higher abstraction.
- template<typename T>
- static T MultiplyByScale(const T& v, const ColumnType& t) {
- DCHECK(t.type == TYPE_DECIMAL);
- return MultiplyByScale(v, t.scale);
+ // Helper function that checks for multiplication overflow. We only check for overflow
+ // if may_overflow is false.
+ template <typename T>
+ static T SafeMultiply(T a, T b, bool may_overflow) {
+ T result = a * b;
+ DCHECK(may_overflow || a == 0 || result / a == b);
+ return result;
}
template<typename T>
- static T MultiplyByScale(const T& v, int scale) {
- return v * GetScaleMultiplier<T>(scale);
+ static T MultiplyByScale(const T& v, int scale, bool may_overflow) {
+ T multiplier = GetScaleMultiplier<T>(scale);
+ DCHECK(multiplier > 0);
+ return SafeMultiply(v, multiplier, may_overflow);
}
template<typename T>