You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Paul Rogers (Code Review)" <ge...@cloudera.org> on 2018/11/29 08:07:50 UTC
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12007
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring
......................................................................
IMPALA-7902: NumericLiteral fixes, refactoring
The work to clean up the rewriter logic must start with a stable AST,
which must start with sprucing up some issues with the leaf nodes. This
CR tackles the NumericLiteral used to hold numbers.
IMPALA-7896: Literals should not need explicit analyze step
Partial fix: removes the need to analyze a numeric literal: analyze() is
a no-op. This eliminates the need to do a "fake" analysis with a null
analyzer: numeric literals are now created analyzed. This is useful
because the catalog module creates numeric literals outside of a query
(and outside of an analyzer.)
Since a literal is (mostly) immutable (except for type), the constructor
now sets the type and cost, then marks the node as analyzed. A later
call to analyze() has nothing to do.
Code that created and dummy-analyzed numeric literals changed to use
static create() methods resulting in simpler literal creation.
IMPALA-7886: NumericLiteral constructor fails to round values to
Decimal type
IMPALA-7887: NumericLiteral fails to detect numeric overflow
IMPALA-7888: Incorrect NumericLiteral overflow checks for FLOAT,
DOUBLE
IMPALA-7891: Analyzer does not detect numeric overflow in CAST
IMPALA-7894: Parser does not catch double overflow
These are all caused by the somewhat messy state of the numeric
check code after years of incremental changes. This patch centralizes
all checks into a series of constants and methods so that they are
uniform. All values are set in constructors which now all uniformly
check that the value is legal for the type. (Explicit) cast operations
verify that the cast is valid. Multiple semi-parallel versions of the
same logic is replaced by calls to a single implementation.
The numeric checks now follow the SQL standard which says that
implementations should fail if a cast would trucate most significant
digits, but round when truncating the least significant.
In this commit, the rules apply only to numeric literals created
outside of the parser. The existing analysis rules are unchanged.
IMPALA-7865: Repeated type widening of arithmetic expressions
Partial fix. Replaces the "is explicit cast" flag in the numeric literal
with the explicit type. This allows reseting an implicit type back to
the explciit type if an arithmetic expression is analyzed multiple
times. A later patch will feed this type information into the type
inference mechanism to complete the fix.
Finally, adds a set of new exceptions that begin to unify error
reporting. These handle casts (SqlCastException), value validation
(InvalidValueException) and unsupported features
(UnsupportedFeatureException.) These all derive from AnalysisException
for backward compatibility. Tests use the new exceptions to check for
expected errors rather than parsing text strings (which tend to
change.)
Testing:
* Added unit tests just for numeric literals. Refactored code to
simplify the tests.
* Added a test case for the obscure case in Decimal V1 of an implicit
cast overflow.
* Ran all FE tests.
Change-Id: I484600747b2871d3a6fe9153751973af9a8534f2
Fix
Change-Id: I75836774271058a047e8bef3ac8ef9f2b96ee511
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/common/InvalidValueException.java
A fe/src/main/java/org/apache/impala/common/SqlCastException.java
A fe/src/main/java/org/apache/impala/common/UnsupportedFeatureException.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java
M fe/src/test/java/org/apache/impala/catalog/HdfsPartitionTest.java
15 files changed, 1,063 insertions(+), 170 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/12007/1
--
To view, visit http://gerrit.cloudera.org:8080/12007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75836774271058a047e8bef3ac8ef9f2b96ee511
Gerrit-Change-Number: 12007
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/12007 )
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring
......................................................................
Abandoned
Wrong branch
--
To view, visit http://gerrit.cloudera.org:8080/12007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I75836774271058a047e8bef3ac8ef9f2b96ee511
Gerrit-Change-Number: 12007
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12007 )
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring
......................................................................
Patch Set 1:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@668
PS1, Line 668: paramExprs.add(NumericLiteral.create(window_.getRightBoundary().getOffsetValue(),
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java
File fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java:
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@247
PS1, Line 247: assertFalse(NumericLiteral.isOverflow(BigDecimal.valueOf(Float.MIN_VALUE), Type.FLOAT));
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@259
PS1, Line 259: assertFalse(NumericLiteral.isOverflow(BigDecimal.valueOf(Double.MIN_VALUE), Type.DOUBLE));
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@269
PS1, Line 269: assertFalse(NumericLiteral.isOverflow(new BigDecimal(genDecimal(10,5)), Type.DECIMAL));
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@270
PS1, Line 270: assertFalse(NumericLiteral.isOverflow(new BigDecimal(genDecimal(10,5)), Type.DECIMAL));
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@271
PS1, Line 271: assertFalse(NumericLiteral.isOverflow(new BigDecimal(genDecimal(ScalarType.MAX_PRECISION, 0)), Type.DECIMAL));
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@272
PS1, Line 272: assertFalse(NumericLiteral.isOverflow(new BigDecimal(genDecimal(0, ScalarType.MAX_PRECISION)), Type.DECIMAL));
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@273
PS1, Line 273: assertTrue(NumericLiteral.isOverflow(new BigDecimal(genDecimal(ScalarType.MAX_PRECISION + 1, 0)), Type.DECIMAL));
line too long (117 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@274
PS1, Line 274: assertTrue(NumericLiteral.isOverflow(new BigDecimal(genDecimal(ScalarType.MAX_PRECISION + 1, 1)), Type.DECIMAL));
line too long (117 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@275
PS1, Line 275: assertTrue(NumericLiteral.isOverflow(new BigDecimal(genDecimal(0, ScalarType.MAX_PRECISION + 1)), Type.DECIMAL));
line too long (117 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@438
PS1, Line 438: result = NumericLiteral.convertValue(BigDecimal.ZERO, ScalarType.createDecimalType(2, 2));
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java:
http://gerrit.cloudera.org:8080/#/c/12007/1/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java@235
PS1, Line 235: Assert.assertTrue(e.getMessage().contains("Value 11.1 cannot be cast to type DECIMAL(1,0)"));
line too long (99 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75836774271058a047e8bef3ac8ef9f2b96ee511
Gerrit-Change-Number: 12007
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Nov 2018 08:08:37 +0000
Gerrit-HasComments: Yes