[Impala-ASF-CR] IMPALA-2422: Fix escaping in the LIKE clause
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10910 ) Change subject: IMPALA-2422: Fix escaping in the LIKE clause .. IMPALA-2422: Fix escaping in the LIKE clause There are two stages to processing a like clause. First, we determine if it is possible to convert the expression to a simpler function, such as StartsWith() or EndsWith(). If not, then we use a Regex libarary to compute the result. There was a problem in the logic that determines if it is possible to use a simpler function. It did not take into account escape characters. For example, "abc\%" was incorrectly converted to StartsWith("abc\"). There was another problem. We always unescaped strings in the frontend. The RE2 regex function also unescapes the regex before proceeding. So regexes were unescaped twice, which caused some ambiguity. For example, "abc\%" and "abc\\%" are unescaped in the frontend and the same pattern, "abc\%" is sent to the backend. The backend could not decide if this pattern is an exact or prefix match. To fix this problem, we avoid unescaping regex pattens in the frontend. Testing: -Added expr tests. Change-Id: I553412318525820a36d2f401aa7e93958d22f70e --- M be/src/exprs/expr-test.cc M be/src/exprs/like-predicate.cc M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java 5 files changed, 68 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10910/2 -- To view, visit http://gerrit.cloudera.org:8080/10910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I553412318525820a36d2f401aa7e93958d22f70e Gerrit-Change-Number: 10910 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-2422: Fix escaping in the LIKE clause
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10910 ) Change subject: IMPALA-2422: Fix escaping in the LIKE clause .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/10910/1/be/src/exprs/like-predicate.cc File be/src/exprs/like-predicate.cc: http://gerrit.cloudera.org:8080/#/c/10910/1/be/src/exprs/like-predicate.cc@265 PS1, Line 265:StringValue tmp_val = StringValue::FromStringVal(val); > ws Oops , this was temporary garbage that I forgot to remove (same as below). http://gerrit.cloudera.org:8080/#/c/10910/1/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java: http://gerrit.cloudera.org:8080/#/c/10910/1/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@132 PS1, Line 132: StringLiteral > basic question: what happens with the interaction between constant folding If the second child becomes a string literal due to constant folding, then the NeedsUnescaping will not be flipped to false here. And I think this is OK. I added a test for this. http://gerrit.cloudera.org:8080/#/c/10910/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/10910/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@a304 PS1, Line 304: > basic question: this changes the test. are inputs expected to change? just Not quite sure what you mean. The test is the same as before, except we remove one level of escaping. Before, with four slashes the escaping worked as follows: 1. The first level escaped the Java escaping 2. The second level escaped the FE escaping. So the BE would receive this: "\_" After my change, the backend receives the same thing because the FE escaping is eliminated. -- To view, visit http://gerrit.cloudera.org:8080/10910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I553412318525820a36d2f401aa7e93958d22f70e Gerrit-Change-Number: 10910 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 12 Jul 2018 02:43:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10898 ) Change subject: IMPALA-7211: Fix the between predicate for decimals .. IMPALA-7211: Fix the between predicate for decimals Before this patch, some queries would fail where the inputs to the between predicate were decimal types that are not compatible with each other. We would needlessly cast them to the same type in the FE. This was not necessary because we are able to handle decimals of different types in the backend already for this predicate. This patch should even result in a slight performance improvement. Testing: - Added some tests to AnalyzeExprsTest. Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 3 files changed, 62 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/4 -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10898 ) Change subject: IMPALA-7211: Fix the between predicate for decimals .. Patch Set 3: Code-Review+2 Rebased. Forwarding the +2 -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 11 Jul 2018 06:05:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10898 ) Change subject: IMPALA-7211: Fix the between predicate for decimals .. IMPALA-7211: Fix the between predicate for decimals Before this patch, some queries would fail where the inputs to the between predicate were decimal types that are not compatible with each other. We would needlessly cast them to the same type in the FE. This was not necessary because we are able to handle decimals of different types in the backend already for this predicate. This patch should even result in a slight performance improvement. Testing: - Added some tests to AnalyzeExprsTest. Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 3 files changed, 51 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/3 -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10898 ) Change subject: IMPALA-7211: Fix the between predicate for decimals .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10898/2/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java: http://gerrit.cloudera.org:8080/#/c/10898/2/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@54 PS2, Line 54: that > nit: then Done -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 11 Jul 2018 05:59:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2422: Fix escaping in the LIKE clause
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10910 Change subject: IMPALA-2422: Fix escaping in the LIKE clause .. IMPALA-2422: Fix escaping in the LIKE clause There are two stages to processing a like clause. First, we determine if it is possible to convert the expression to a simpler function, such as StartsWith() or EndsWith(). If not, then we use a Regex libarary to compute the result. There was a problem in the logic that determines if it is possible to use a simpler function. It did not take into account escape characters. For example, "abc\%" was incorrectly converted to StartsWith("abc\"). There was another problem. We always unescaped strings in the frontend. The RE2 regex function also unescapes the regex before proceeding. So regexes were unescaped twice, which caused some ambiguity. For example, "abc\%" and "abc\\%" are unescaped in the frontend and the same pattern, "abc\%" is sent to the backend. The backend could not decide if this pattern is an exact or prefix match. To fix this problem, we avoid unescaping regex pattens in the frontend. Testing: -Added expr tests. Change-Id: I553412318525820a36d2f401aa7e93958d22f70e --- M be/src/exprs/expr-test.cc M be/src/exprs/like-predicate.cc M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java 5 files changed, 77 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10910/1 -- To view, visit http://gerrit.cloudera.org:8080/10910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I553412318525820a36d2f401aa7e93958d22f70e Gerrit-Change-Number: 10910 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10898 ) Change subject: IMPALA-7211: Fix the between predicate for decimals .. IMPALA-7211: Fix the between predicate for decimals Before this patch, some queries would fail where the inputs to the between predicate were decimal types that are not compatible with each other. We would needlessly cast them to the same type in the FE. This was not necessary because we are able to handle decimals of different types in the backend already for this predicate. This patch should even result in a slight performance improvement. Testing: - Added some tests to AnalyzeExprsTest. Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 3 files changed, 51 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/2 -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] PREVIEW: IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10898 ) Change subject: PREVIEW: IMPALA-7211: Fix the between predicate for decimals .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java: http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@60 PS1, Line 60: ( > remove or should something be added here? Done http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@62 PS1, Line 62: noFloats > worth it to add a comment about why floats are specially handled? Done http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@72 PS1, Line 72: for(int i = 0; i < children_.size(); ++i) { > if I read this correctly, for children with types decimal, int, int, we'll Added a comment -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 10 Jul 2018 23:48:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10888 ) Change subject: IMPALA-7260: Fix decimal binary predicates .. IMPALA-7260: Fix decimal binary predicates When casting the inputs to a function call, we would try to cast non decimal numbers to a specific decimal type even though this is not necessary. The specific decimal type would be calculated by looking at all the inputs. It was possible for this calculation to fail due to some decimal types being incompatible with each other. We solve the problem by casting the non-decimal numerical inputs to a generic getMinResolutionDecimal(). Testing: - Added some analysis tests. Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 2 files changed, 23 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10888/4 -- To view, visit http://gerrit.cloudera.org:8080/10888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 Gerrit-Change-Number: 10888 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10882 ) Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d Gerrit-Change-Number: 10882 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Tue, 10 Jul 2018 21:58:26 + Gerrit-HasComments: No
[Impala-ASF-CR] PREVIEW: IMPALA-7211: Fix the between predicate for decimals
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10898 Change subject: PREVIEW: IMPALA-7211: Fix the between predicate for decimals .. PREVIEW: IMPALA-7211: Fix the between predicate for decimals Before this patch, some queries would fail where the inputs to the between predicate were decimal types that are not compatible with each other. We would needlessly cast them to the same type in the FE. This was not necessary because we are able to handle decimals of different types in the backend already for this predicate. This patch should even result in a slight performance improvement. Testing: - Added some tests to AnalyzeExprsTest. Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 3 files changed, 43 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/1 -- To view, visit http://gerrit.cloudera.org:8080/10898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312 Gerrit-Change-Number: 10898 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10882 ) Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2286 PS3, Line 2286: Collections.sort(sortedExprs, new Comparator() { > The code actually puts all the decimals at the beginning. If we handle non- Ok, I think it's fine the way it is written now. http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2451 PS3, Line 2451: 2 as decimal(38, 37) > I have test cases for that in L2464 and L2491. Yeah, but it would be better to have decimals that are incompatible with each other here also. This test case is not adding anything new the way it is written now. Many of these tests would pass even without this patch. -- To view, visit http://gerrit.cloudera.org:8080/10882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d Gerrit-Change-Number: 10882 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Tue, 10 Jul 2018 03:25:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10888 ) Change subject: IMPALA-7260: Fix decimal binary predicates .. IMPALA-7260: Fix decimal binary predicates When casting the inputs to a function call, we would try to cast non decimal numbers to a specific decimal type even though this is not necessary. The specific decimal type would be calculated by looking at all the inputs. It was possible for this calculation to fail due to some decimal types being incompatible with each other. We solve the problem by casting the non-decimal numerical inputs to a generic getMinResolutionDecimal(). Testing: - Added some analysis tests. Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 2 files changed, 18 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10888/2 -- To view, visit http://gerrit.cloudera.org:8080/10888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 Gerrit-Change-Number: 10888 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10888 ) Change subject: IMPALA-7260: Fix decimal binary predicates .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@265 PS1, Line 265: AnalyzesOk("select cast(1 as decimal(38,37)) " + operator + " cast(2 as bigint)"); > add a comment explaining what this is trying to test. Done http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2418 PS1, Line 2418: testDecimalExpr(decimal_5_5 + " + cast(1 as tinyint)", > these tests look more specific than the added ones since the return type is No we don't get lucky here. Arithmetic operations go through a different code path. http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2496 PS1, Line 2496: public void TestDecimalFunctions() throws AnalysisException { > worth it to add cases here for non-binary predicates? I don't think so. This patch only affects the behavior of decimal binary predicates. -- To view, visit http://gerrit.cloudera.org:8080/10888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 Gerrit-Change-Number: 10888 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 09 Jul 2018 22:12:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10888 Change subject: IMPALA-7260: Fix decimal binary predicates .. IMPALA-7260: Fix decimal binary predicates When casting the inputs to a function call, we would try to cast non decimal numbers to a specific decimal type even though this is not necessary. The specific decimal type would be calculated by looking at all the inputs. It was possible for this calculation to fail due to some decimal types being incompatible with each other. We solve the problem by casting the non-decimal numerical inputs to a generic getMinResolutionDecimal(). Testing: - Added some analysis tests. Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java 2 files changed, 14 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10888/1 -- To view, visit http://gerrit.cloudera.org:8080/10888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 Gerrit-Change-Number: 10888 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10882 ) Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2286 PS3, Line 2286: Collections.sort(sortedExprs, new Comparator() { It looks like we are sorting here in order to put the decimals at the end of the list. Do you think it would be simpler if we just did 2 passes over exprs instead (without creating a new list)? Where we handle the non-decimals in the first pass and the decimals in the second pass. http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2451 PS3, Line 2451: 2 as decimal(38, 37) One should be decimal(38,37) and the other one decimal(38,1) - to make sure that the decimals are not compatible with each other. Here and elsewhere. -- To view, visit http://gerrit.cloudera.org:8080/10882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d Gerrit-Change-Number: 10882 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 06 Jul 2018 23:25:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10859 ) Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test .. Patch Set 1: Yeah, those should probably be merged before this change, because our build would get broken. Alternatively, we can merge this change, but disable this test with an xfail. The test can be re-enabled in the IMPALA-7242 patch by whoever fixes the Jira. -- To view, visit http://gerrit.cloudera.org:8080/10859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0 Gerrit-Change-Number: 10859 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 06 Jul 2018 00:45:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10859 Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test .. IMPALA-7202: Add width_bucket() to the decimal fuzz test In this patch we include the newly added width_bucket() function in the decimal fuzz test. Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0 --- M tests/query_test/test_decimal_fuzz.py 1 file changed, 57 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/10859/1 -- To view, visit http://gerrit.cloudera.org:8080/10859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0 Gerrit-Change-Number: 10859 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7236: Fix the parsing of ALLOW ERASURE CODED FILES
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10857 ) Change subject: IMPALA-7236: Fix the parsing of ALLOW_ERASURE_CODED_FILES .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife1e791541e3f4fed6bec00945390c7d7681e824 Gerrit-Change-Number: 10857 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Tue, 03 Jul 2018 18:49:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 21: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 21 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Sat, 30 Jun 2018 01:25:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 20: (1 comment) http://gerrit.cloudera.org:8080/#/c/6023/20/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/20/be/src/exprs/math-functions-ir.cc@565 PS20, Line 565: ctx->SetError("Encountered division by 0"); > clang fails with division by 0. Since we check for This is not the right way to deal with this. I looked at it again, and I really don't think it's possible for y to be zero. This needless check will make the code slower. Include this comment on the line that is causing the problem to suppress that clang error: // NOLINT: clang-tidy thinks y may equal zero here. You just need to put this on line 584 I think -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 20 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 29 Jun 2018 21:54:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7102 (Part 1): Disable reading of erasure coding by default
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10646 ) Change subject: IMPALA-7102 (Part 1): Disable reading of erasure coding by default .. Patch Set 4: Code-Review+2 Fixed a typo in the test file. Forwarding the +2. -- To view, visit http://gerrit.cloudera.org:8080/10646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 Gerrit-Change-Number: 10646 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Adrian Ng Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 29 Jun 2018 20:10:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7102 (Part 1): Disable reading of erasure coding by default
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10646 ) Change subject: IMPALA-7102 (Part 1): Disable reading of erasure coding by default .. IMPALA-7102 (Part 1): Disable reading of erasure coding by default In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that allows us to enable or disable the support of erasure coded files. Even though Impala should be able to handle HDFS erasure coded files already, this feature hasn't been tested thoroughly yet. Also, Impala lacks metrics, observability and DDL commands related to erasure coding. This is a query option instead of a startup flag because we want to make it possible for advanced users to enable the feature. We may also need a follow on patch to also disable the write path with this flag. Cherry-picks: not for 2.x Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 --- M be/src/service/query-options.cc M be/src/service/query-options.h M bin/run-all-tests.sh M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M testdata/bin/create-load-data.sh A testdata/workloads/functional-query/queries/QueryTest/hdfs-erasure-coding.test M tests/common/custom_cluster_test_suite.py M tests/common/skip.py M tests/query_test/test_observability.py M tests/query_test/test_scanners.py 12 files changed, 61 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/10646/4 -- To view, visit http://gerrit.cloudera.org:8080/10646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 Gerrit-Change-Number: 10646 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Adrian Ng Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7102(Part 1): Disable reading of erasure coding by default
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10646 ) Change subject: IMPALA-7102(Part 1): Disable reading of erasure coding by default .. IMPALA-7102(Part 1): Disable reading of erasure coding by default In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that allows us to enable or disable the support of erasure coded files. Even though Impala should be able to handle HDFS erasure coded files already, this feature hasn't been tested thoroughly yet. Also, Impala lacks metrics, observability and DDL commands related to erasure coding. This is a query option instead of a startup flag because we want to make it possible for advanced users to enable the feature. We may also need a follow on patch to also disable the write path with this flag. Cherry-picks: not for 2.x Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 --- M be/src/service/query-options.cc M be/src/service/query-options.h M bin/run-all-tests.sh M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M testdata/bin/create-load-data.sh A testdata/workloads/functional-query/queries/QueryTest/hdfs-erasure-coding.test M tests/common/custom_cluster_test_suite.py M tests/common/skip.py M tests/query_test/test_observability.py M tests/query_test/test_scanners.py 12 files changed, 61 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/10646/2 -- To view, visit http://gerrit.cloudera.org:8080/10646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 Gerrit-Change-Number: 10646 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Adrian Ng Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7102: Disable support of erasure coding by default
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10646 ) Change subject: IMPALA-7102: Disable support of erasure coding by default .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10646/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10646/1//COMMIT_MSG@9 PS1, Line 9: In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that > I had a high-level question: what's the rationale for making it a query opt Updated the commit message. The feature should work in principle and has been tested to a certain extent already and it works. It wouldn't hurt to make it possible for advanced users to be able to enabled the feature if they know what they are doing. http://gerrit.cloudera.org:8080/#/c/10646/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/10646/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@786 PS1, Line 786: "The query involves scanning an erasure coded file %s located in %s. " + Updated the message -- To view, visit http://gerrit.cloudera.org:8080/10646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 Gerrit-Change-Number: 10646 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Adrian Ng Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 27 Jun 2018 00:01:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7149: Disable some tests in the EC build
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10804 Change subject: IMPALA-7149: Disable some tests in the EC build .. IMPALA-7149: Disable some tests in the EC build We temporarily disable the resource limits tests in the EC build to make it pass. We also disable the tests marked with "tuned_for_minicluster" in the EC build. Cherry-picks: not for 2.x. Change-Id: I0975b1a28b318625f853b612bdfea3a8adcd776e --- M tests/common/skip.py M tests/query_test/test_resource_limits.py 2 files changed, 4 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/10804/1 -- To view, visit http://gerrit.cloudera.org:8080/10804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0975b1a28b318625f853b612bdfea3a8adcd776e Gerrit-Change-Number: 10804 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 16 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 22 Jun 2018 23:46:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6642 (Part 2): clean up start-impala-cluster.py
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10780 ) Change subject: IMPALA-6642 (Part 2): clean up start-impala-cluster.py .. IMPALA-6642 (Part 2): clean up start-impala-cluster.py We clean up start-impala-cluster.py in general in this patch by using logging instead of "print" and formatting strings using the format() function. We make sure to include a timestamp in each log message in order to make it easier to debug failures in custom cluster tests that happen when starting the cluster. Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 --- M bin/start-impala-cluster.py M tests/common/impala_service.py 2 files changed, 147 insertions(+), 83 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/10780/2 -- To view, visit http://gerrit.cloudera.org:8080/10780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 Gerrit-Change-Number: 10780 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10780 ) Change subject: IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10780/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/10780/1/bin/start-impala-cluster.py@460 PS1, Line 460: print 'Error starting cluster: {0}'.format(e) > If you switch to logging, this becomes just "logging.exception("Error start Done -- To view, visit http://gerrit.cloudera.org:8080/10780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 Gerrit-Change-Number: 10780 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Fri, 22 Jun 2018 23:02:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10780 Change subject: IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py .. IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py We add some timestamps to the output of start-impala-cluster.py in order to make it easier to debug failures in custom cluster tests that happen when starting the cluster. Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 --- M bin/start-impala-cluster.py M tests/common/impala_service.py 2 files changed, 22 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/10780/1 -- To view, visit http://gerrit.cloudera.org:8080/10780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 Gerrit-Change-Number: 10780 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 15: Also, I agree with Tim that it's a good idea to add this to the fuzz test. I think that it is ok to do that in a separate commit. -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 15 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Wed, 20 Jun 2018 22:57:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 15: I already let Anuj know offline that it's ok to remove the precondition. -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 15 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Wed, 20 Jun 2018 22:55:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7149: Skip q7 in test mem usage scaling in erasure coding build
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10647 Change subject: IMPALA-7149: Skip q7 in test_mem_usage_scaling in erasure coding build .. IMPALA-7149: Skip q7 in test_mem_usage_scaling in erasure coding build The test is flaky in the erasure coding build. Let's disable it for now. Change-Id: Ic9a34a91eef40e1da9c7134cfb7054006d9115de --- M tests/query_test/test_mem_usage_scaling.py 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/10647/1 -- To view, visit http://gerrit.cloudera.org:8080/10647 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic9a34a91eef40e1da9c7134cfb7054006d9115de Gerrit-Change-Number: 10647 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7102: Disable support of erasure coding by default
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10646 Change subject: IMPALA-7102: Disable support of erasure coding by default .. IMPALA-7102: Disable support of erasure coding by default In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that allows us to enable or disable the support of erasure coded files. At this time erasure coding is an experimental feature and is not supported by Impala, so we want to prevent users from using it without explictly enabling it. Cherry-picks: not for 2.x Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 --- M be/src/service/query-options.cc M be/src/service/query-options.h M bin/run-all-tests.sh M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M testdata/bin/create-load-data.sh A testdata/workloads/functional-query/queries/QueryTest/hdfs-erasure-coding.test M tests/common/custom_cluster_test_suite.py M tests/common/skip.py M tests/query_test/test_observability.py M tests/query_test/test_scanners.py 12 files changed, 64 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/10646/1 -- To view, visit http://gerrit.cloudera.org:8080/10646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3 Gerrit-Change-Number: 10646 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-6642 (Part 1): Fix the log directory for test redaction.py
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10599 Change subject: IMPALA-6642 (Part 1): Fix the log directory for test_redaction.py .. IMPALA-6642 (Part 1): Fix the log directory for test_redaction.py We don't get the Impalad logs when running the test_redaction.py custom cluster test because the test sets the log directory to /tmp, and we do not save the logs from there. In this patch we fix the problem by correctly setting the logging directory in that test. Change-Id: I82123ceea8633a5a0831019b810239fef3cd3212 --- M tests/custom_cluster/test_redaction.py 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/10599/1 -- To view, visit http://gerrit.cloudera.org:8080/10599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I82123ceea8633a5a0831019b810239fef3cd3212 Gerrit-Change-Number: 10599 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7079: Disable the multiple blocks test in erasure coding build
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10521 Change subject: IMPALA-7079: Disable the multiple blocks test in erasure coding build .. IMPALA-7079: Disable the multiple blocks test in erasure coding build The test is currently failing in erasure coding build, so disable it to make the build pass. Change-Id: I00af0914d907b8dcff69f687f71239e76b6ff335 --- M tests/query_test/test_scanners.py 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/10521/1 -- To view, visit http://gerrit.cloudera.org:8080/10521 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I00af0914d907b8dcff69f687f71239e76b6ff335 Gerrit-Change-Number: 10521 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/6023/15/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/6023/15/fe/src/main/java/org/apache/impala/analysis/Expr.java@490 PS15, Line 490: result = ((ScalarType)result).getMinResolutionDecimal(); Why do we need this? -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 15 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 22 May 2018 00:17:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10413 ) Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84 Gerrit-Change-Number: 10413 Gerrit-PatchSet: 5 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Mon, 21 May 2018 21:33:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7039: Ignore the port in HBase planner tests
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10459 Change subject: IMPALA-7039: Ignore the port in HBase planner tests .. IMPALA-7039: Ignore the port in HBase planner tests Before this patch, we used to check the HBase port in the HBase planner tests. This caused a failure when HBase was running on a different port than expected. We fix the problem in this patch by not checking the HBase port. Testing: ran the FE tests and they passed. Change-Id: I8eb7628061b2ebaf84323b37424925e9a64f70a0 --- M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test 3 files changed, 20 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/10459/1 -- To view, visit http://gerrit.cloudera.org:8080/10459 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8eb7628061b2ebaf84323b37424925e9a64f70a0 Gerrit-Change-Number: 10459 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10413 ) Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/10413/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/10413/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@129 PS4, Line 129: static FileDescriptor createEc(FileStatus fileStatus, BlockLocation[] blockLocations, The logic here seems almost identical to the create() function. Maybe modify the create() function to accept the isEC argument instead of creating a new function? -- To view, visit http://gerrit.cloudera.org:8080/10413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84 Gerrit-Change-Number: 10413 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Fri, 18 May 2018 23:05:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10413 ) Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10413/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10413/3//COMMIT_MSG@12 PS3, Line 12: Mention how this patch was tested. Did you run the exhaustive build? http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py@29 PS3, Line 29: IS_ISILON, : IS_LOCAL, : IS_HDFS, : IS_S3, : IS_ADLS, : SECONDARY_FILESYSTEM, : IS_EC This should be sorted alphabetically http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py@150 PS3, Line 150: doesn't work. "do not work" http://gerrit.cloudera.org:8080/#/c/10413/3/tests/util/filesystem_utils.py File tests/util/filesystem_utils.py: http://gerrit.cloudera.org:8080/#/c/10413/3/tests/util/filesystem_utils.py@33 PS3, Line 33: os.getenv("ERASURE_CODING") We want to check if ERASURE_CODING == true here. -- To view, visit http://gerrit.cloudera.org:8080/10413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84 Gerrit-Change-Number: 10413 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Fri, 18 May 2018 21:57:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. Patch Set 5: Code-Review+2 Added Cherry-picks: not for 2.x Forwarding the +2. Thanks, Phil! -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 5 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Fri, 04 May 2018 21:42:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. IMPALA-6949: Add the option to start the minicluster with EC enabled In this patch we add the "ERASURE_CODING" enviornment variable. If we enable it, a cluster with 5 data nodes will be created during data loading and HDFS will be started with erasure coding enabled. Testing: I ran the core build, and verified that erasure coding gets enabled in HDFS. Many of our EE tests failed however. Cherry-picks: not for 2.x Change-Id: I397aed491354be21b0a8441ca671232dca25146c --- M bin/impala-config.sh M bin/run-all-tests.sh M testdata/bin/create-load-data.sh M testdata/bin/setup-hdfs-env.sh M testdata/cluster/admin M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl 6 files changed, 41 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/5 -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 5 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. IMPALA-6949: Add the option to start the minicluster with EC enabled In this patch we add the "ERASURE_CODING" enviornment variable. If we enable it, a cluster with 5 data nodes will be created during data loading and HDFS will be started with erasure coding enabled. Testing: I ran the core build, and verified that erasure coding gets enabled in HDFS. Many of our EE tests failed however. Change-Id: I397aed491354be21b0a8441ca671232dca25146c --- M bin/impala-config.sh M bin/run-all-tests.sh M testdata/bin/create-load-data.sh M testdata/bin/setup-hdfs-env.sh M testdata/cluster/admin M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl 6 files changed, 41 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/4 -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh@450 PS3, Line 450: elif [ "${TARGET_FILESYSTEM}" == "hdfs" ]; then > I'm trying this out and found that == don't work with zsh. Done http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh@451 PS3, Line 451: if [[ "${ERASURE_CODING}" = true ]]; then > I think we should error out or warn here if MINICLUSTER_PROFILE < 3. I.e., Done http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh File testdata/bin/setup-hdfs-env.sh: http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh@80 PS3, Line 80: HDFS_EC_PATH > Please add: I agree. I don't think it makes sense to unset it here, because it's not going to do anything. Removed. http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl File testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl: http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl@26 PS2, Line 26: cloudera.erasure_coding.enabled > Add: Done -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Fri, 04 May 2018 01:40:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh File testdata/bin/setup-hdfs-env.sh: http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh@80 PS3, Line 80: HDFS_EC_PATH This should be HDFS_ERASURECODE_PATH -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Thu, 03 May 2018 20:35:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. IMPALA-6949: Add the option to start the minicluster with EC enabled In this patch we add the "ERASURE_CODING" enviornment variable. If we enable it, a cluster with 5 data nodes will be created during data loading and HDFS will be started with erasure coding enabled. Testing: I ran the core build, and verified that erasure coding gets enabled in HDFS. Many of our EE tests failed however. Change-Id: I397aed491354be21b0a8441ca671232dca25146c --- M bin/impala-config.sh M bin/run-all-tests.sh M testdata/bin/setup-hdfs-env.sh M testdata/cluster/admin M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl 5 files changed, 28 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/3 -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. Patch Set 2: (5 comments) Yes, nested data loading succeeds every time I tried it. http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh@449 PS2, Line 449: elif [ "${TARGET_FILESYSTEM}" = "hdfs-ec" ]; then > By making this its own TARGET_FILESYSTEM, you end up disabling a bunch of t I don't think it makes sense to make erasure coding its own file system. I made a separate flag for it. http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh@454 PS2, Line 454: echo "Valid values are: hdfs, isilon, s3, local" > Please update? No need to update any more. http://gerrit.cloudera.org:8080/#/c/10275/2/bin/run-all-tests.sh File bin/run-all-tests.sh: http://gerrit.cloudera.org:8080/#/c/10275/2/bin/run-all-tests.sh@71 PS2, Line 71: FE_TEST=false > Add a comment about why? Done http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/bin/setup-hdfs-env.sh File testdata/bin/setup-hdfs-env.sh: http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/bin/setup-hdfs-env.sh@80 PS2, Line 80: hdfs ec -unsetPolicy -path "${HDFS_EC_PATH:=/test-warehouse/}" > Do you know what this does underneath the covers? Do we need to block while This does not convert the existing files in the directory. It only affects the files that will be placed in the directory in the future. This is why it is possible to have some files erasure coded and some not in the same directory. http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl File testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl: http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl@26 PS2, Line 26: cloudera.erasure_coding.enabled > What's this for? This is needed because running the following command hdfs ec -enablePolicy -policy RS-3-2-1024k Causes this error: RemoteException: enableErasureCodingPolicy is not allowed because cloudera.erasure_coding.enabled=false -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang Gerrit-Comment-Date: Wed, 02 May 2018 21:23:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10275 ) Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. IMPALA-6949: Add the option to start the minicluster with EC enabled In this patch we add the "hdfs-ec" target file system option. If we enable it, a cluster with 5 data nodes will be created during data loading and HDFS will be started with erasure coding enabled. Testing: I ran the core build and the majority of the EE tests passed. Change-Id: I397aed491354be21b0a8441ca671232dca25146c --- M bin/impala-config.sh M bin/run-all-tests.sh M buildall.sh M testdata/bin/setup-hdfs-env.sh M testdata/cluster/admin M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl 6 files changed, 24 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/2 -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10275 Change subject: IMPALA-6949: Add the option to start the minicluster with EC enabled .. IMPALA-6949: Add the option to start the minicluster with EC enabled In this patch we add the "hdfs-ec" target file system option. If we enable it, a cluster with 5 data nodes will be created during data loading and HDFS will be started with erasure coding enabled. Change-Id: I397aed491354be21b0a8441ca671232dca25146c --- M bin/impala-config.sh M bin/run-all-tests.sh M buildall.sh M testdata/bin/setup-hdfs-env.sh M testdata/cluster/admin M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl 6 files changed, 24 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/1 -- To view, visit http://gerrit.cloudera.org:8080/10275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c Gerrit-Change-Number: 10275 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10066 ) Change subject: IMPALA-6522: [DOCS] Document Decimal V2 .. Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml File docs/topics/impala_decimal.xml: http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@50 PS10, Line 50: This data type is suitable for financial and other arithmetic calculations where the We should have a better introduction. We would not be comparing it to FLOAT. We should not mention that it's suitable for financial purposes. How about this: Precision is the maximum number of decimal digits and scale is the number of digits to the right of the decimal point. The data type is useful for storing and doing operations on precise decimal values. http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@56 PS10, Line 56: The key differences between this version of DECIMAL and the previous We should move this table to the bottom. Instead of calling it DECIMAL_V1 and DECIMAL_v2 we should say "Decimal in Impala 2.x" and "Decimal in Impala 3.x". Say that the old behavior can be enabled by disabling the query option "set decimal_v2=false" http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293 PS10, Line 293: If the precision of the result would be greater than 38, Impala truncates the result from > truncates and rounds, right Taras? yes, "truncates and rounds" Let's make a separate section called "Decimal Assignment" as Alex mentioned and put UNION in that section. http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668 PS10, Line 668: expressions to DECIMAL as long as the overall number of digits and digits > Taras? Alex is right. This contradicts what it says on line 678. should be "... as number fits within the specified target DECIMAL type without overflow" http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@879 PS10, Line 879: Any values that do not fit within the new precision and scale are returned as > Taras, is this really true? I would have expected that we round or error. No, we do not round. We will error if the query option ABORT_ON_ERROR is set to true. If that option is set to FALSE, we will get a NULL and warning that conversion failed. -- To view, visit http://gerrit.cloudera.org:8080/10066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d Gerrit-Change-Number: 10066 Gerrit-PatchSet: 10 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Sat, 28 Apr 2018 01:09:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Speed up Python dependencies.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10234 ) Change subject: Speed up Python dependencies. .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py File infra/python/deps/pip_download.py: http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@172 PS2, Line 172: if results: > If the import above fails, then results = [] will never execute and this st Nice catch. We can get rid of the "if results:" line -- To view, visit http://gerrit.cloudera.org:8080/10234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30 Gerrit-Change-Number: 10234 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 27 Apr 2018 22:03:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10066 ) Change subject: IMPALA-6522: [DOCS] Document Decimal V2 .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml File docs/topics/impala_decimal.xml: http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@539 PS10, Line 539: OUBLE DOUBLE and FLOAT http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@540 PS10, Line 540: error returns. Maybe add the following. The reasoning is that the range of the floating point types is much wider than the range of the DECIMAL, so not every FLOAT or DOUBLE can be converted to a DECIMAL. However, every DECIMAL can be converted to a double or float with a loss of precision, which is why we allow implicit casting in this case. This is somewhat dangerous, but we made this design choice to match the behavior of other SQL engines. http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@545 PS10, Line 545: Add this: We do not allow this because calculations involving decimals are meant to be precise, so we are strict and require explicit casts. However, if an approximate type like FLOAT is involved, then our behavior is less strict. -- To view, visit http://gerrit.cloudera.org:8080/10066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d Gerrit-Change-Number: 10066 Gerrit-PatchSet: 10 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 27 Apr 2018 21:59:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Speed up Python dependencies.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10234 ) Change subject: Speed up Python dependencies. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30 Gerrit-Change-Number: 10234 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 27 Apr 2018 21:34:17 + Gerrit-HasComments: No
[Impala-ASF-CR] Speed up Python dependencies.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10234 ) Change subject: Speed up Python dependencies. .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py File infra/python/deps/pip_download.py: http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@165 PS1, Line 165: results.append(pool.apply_async(download_package, args=[pkg_name.strip(), pkg_version.strip()])) long line http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@173 PS1, Line 173: x.get() What happens if the download fails for some reason? Will this be detected? -- To view, visit http://gerrit.cloudera.org:8080/10234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30 Gerrit-Change-Number: 10234 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 27 Apr 2018 20:03:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10066 ) Change subject: IMPALA-6522: [DOCS] Document Decimal V2 .. Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml File docs/topics/impala_decimal.xml: http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@526 PS9, Line 526: 38,0 We should be consistent about putting a space after the comma. Either put it everywhere in the DOC or don't put it everywhere. http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@654 PS9, Line 654: is the INT : type with the precision 10. ... because all digits do not fit into DECIMAL(3,0) http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@681 PS9, Line 681: There should be no space before the open brace. Here and elsewhere. -- To view, visit http://gerrit.cloudera.org:8080/10066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d Gerrit-Change-Number: 10066 Gerrit-PatchSet: 9 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 27 Apr 2018 19:21:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE .. IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran a core build which passed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test 34 files changed, 550 insertions(+), 292 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/10 -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 10 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky
[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 ) Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/9930/8/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/9930/8/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@377 PS8, Line 377: Analyzer analyzer, AnalyticWindow.Boundary boundary) throws AnalysisException { > No need to pass analyzer. Done http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@217 PS8, Line 217: Preconditions.checkState(!t0.isDecimal() && !t1.isDecimal()); > remove Done http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/Expr.java@429 PS8, Line 429:* If strictDecimal is true, the function will throw an exception if it is not possible > This isn't accurate because we will still allow decimal->double conversions Done http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@178 PS8, Line 178:* message. > Needs comment for 'strictDecimal' Done http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@433 PS8, Line 433:* is INVALID_TYPE. > Especially these very visible and heavily used functions need a comment for Done http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/Type.java@293 PS8, Line 293:* If strictDecimal is true, only consider casts that result in no loss of information > This is a nice description! I suggest replicating it in more places. Done -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 8 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Thu, 26 Apr 2018 21:07:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE .. IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran the BE and FE tests locally and they all passed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test 34 files changed, 547 insertions(+), 292 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/9 -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 9 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky
[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran the BE and FE tests locally and they all passed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test 34 files changed, 520 insertions(+), 288 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/8 -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 8 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. Patch Set 7: Code-Review+2 Made a minor fix to widetable.py in patch 6. Rebased. Forwarding the +2 from Alex. -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 7 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Tue, 24 Apr 2018 22:33:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Core build passed. Ran an exhaustive build. The errors discovered by the exhaustive build were fixed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/common/widetable.py M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/funct
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Core build passed. Ran an exhaustive build. The errors discovered by the exhaustive build were fixed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/common/widetable.py M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/funct
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Core build passed. Ran an exhaustive build. The errors discovered by the exhaustive build were fixed. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/common/widetable.py M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/funct
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@119 PS4, Line 119: Preconditions.checkState(precision <= MAX_PRECISION); > Pretty sure these should break a few tests. I believe in some places we rel Yes, it broke some tests. Removed. http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2570 PS4, Line 2570: AnalysisContext decimalV1Ctx = createAnalysisCtx(Catalog.DEFAULT_DB); > you can remove the Catalog.DEFAULT_DB arg Done http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2654 PS4, Line 2654: AnalysisContext decimalV1Ctx = createAnalysisCtx("functional"); > Use createAnalysisCtx() without an arg. There's no significance to "functio Done http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@348 PS4, Line 348: if (!expectedPlan.get(0).contains("NotImplementedException") && > Do startsWith() instead of contains() and expect only the exception name an Done -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Tue, 24 Apr 2018 00:15:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran a core build and almost all tests passed. I still need to fix A few tests in PlannerTest.testJoinOrder. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/functional-query/queries/QueryTest/exprs.test M testdata/workloads/functional-query/queries/QueryTest/insert.test M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test M testdata/workloads/functional-query/queries/QueryTe
[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran a core build and almost all tests passed. I still need to fix A few tests in PlannerTest.testJoinOrder. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/functional-query/queries/QueryTest/exprs.test M testdata/workloads/functional-query/queries/QueryTest/insert.test M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test M testdata/workloads/functional-query/queri
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/9930/2/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/9930/2/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@377 PS2, Line 377: Analyzer analyzer, AnalyticWindow.Boundary boundary) throws AnalysisException { > analyzer arg not needed anymore Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@217 PS2, Line 217: boolean strict = analyzer.isDecimalV2() && (t0.isDecimal() || t1.isDecimal()); > remove? Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@423 PS2, Line 423:* Otherwise, if the function signature contains wildcard decimals, each wildcard child > Describe how the decimalV2 argument changes the behavior of this function. Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@451 PS2, Line 451: argTypes.append(childType.toString()); > childType.toSql() Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@453 PS2, Line 453: if (analyzer.isDecimalV2() && digitsBefore + digitsAfter > 38) return Type.INVALID; > I think it's clearer to not call createClippedDecimaltype() for V2 at all b Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java@72 PS2, Line 72: if (decimalV2 && digitsBefore + digitsAfter > 38) return Type.INVALID; > Let's avoid calling createClippedDecimalType() for V2 Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/catalog/Type.java@310 PS2, Line 310: Type t1, Type t2, boolean strict) { > move into previous line Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@91 PS2, Line 91: "Operand types: %s = %s.", eqPred.toSql(), t0.toString(), t1.toString())); > also use toSql() for types Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1838 PS2, Line 1838: AnalyzesOk("select coalesce(1.8, cast(0 as decimal(38,38)))", decimalV1Ctx); > Don't we already have tests for this below in TestDecimalFunctions()? I thi I forgot about those. Added explicit context for v1 and v2 there. http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1840 PS2, Line 1840: "Cannot resolve DECIMAL types of the _impala_builtins.coalesce(DECIMAL(2,1), " + > Should we maybe omit the database when printing the FunctionName? Most of t How do we avoid printing the function name? It gets printed when we call fn.getName(). http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1843 PS2, Line 1843: String query = "with x as ( " + > Why have a WITH clause? A UNION alone should suffice Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2538 PS2, Line 2538: String.format("Cannot resolve DECIMAL precision and scale from NULL type in _impala_builtins.%s function.", alias)); > long lines (here and below) Done http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.ja
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran a core build and almost all tests passed. I still need to fix A few tests in PlannerTest.testJoinOrder. Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test M testdata/workloads/functional-query/queries/QueryTest/exprs.test M testdata/workloads/functional-query/queries/QueryTest/kudu_update.test M testdata/workloads/functional-query/queries/QueryTest/partition-col-types.test M testdata/workloads/functional-query/queries/QueryTest/union.test M testdata/workloads/tpcds/queries/tpcds-decimal_v2-q21.test 41 files changed, 454 insertions(+), 251 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/2 -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageTy
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 ) Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. Patch Set 1: (27 comments) http://gerrit.cloudera.org:8080/#/c/9930/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/9930/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@385 PS1, Line 385: rangeExpr.getType(), orderByElements_.get(0).getExpr().getType(), > Let's change this to be strict regardless of decimal_v2. My understanding i Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@218 PS1, Line 218: false, analyzer.getQueryOptions().isDecimal_v2()); > Since this decimalV2 check is so common, let's add a function in Analyzer d Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@429 PS1, Line 429: protected void castForFunctionCall(boolean ignoreWildcardDecimals, boolean decimal_v2) > decimalV2 (camel-case) Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@440 PS1, Line 440: if (resolvedWildcardType.isInvalid()) { > Check this immediately after L433? Not a good idea because if ignoreWildcardDecimals is true we don't want to throw. http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@442 PS1, Line 442: "Unable to resolve the decimal types of the %s function arguments. " + > Also include the argument types in the error message Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@461 PS1, Line 461: StringBuilder errorMsg = new StringBuilder(); > unused? Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@482 PS1, Line 482: if (result.isNull()) { > Let's consistently throw either in this function or in the caller. The call Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Function.java File fe/src/main/java/org/apache/impala/catalog/Function.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Function.java@217 PS1, Line 217: other.argTypes_[i], this.argTypes_[i], strict, false)) { > This looks like a case that shows that the decimalV2 arg and 'strict' shoul Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@141 PS1, Line 141: public static ScalarType createClippedDecimalType( > It feels cleaner to me to move the decimalV2 check to the callers and not i Done, checking at callers. http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@381 PS1, Line 381: Preconditions.checkState(scale_ <= 38); > <= MAX_PRECISION Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@393 PS1, Line 393: Preconditions.checkState(scale_ <= 38); > <= MAX_PRECISION Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java@292 PS1, Line 292:* If strict is true, only consider casts that result in no loss of precision. > Why is the decimalV2 case different than the strict case? Done http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java@296 PS1, Line 296: Type t1, Type t2, boolean strict, boolean decimal_v2) { > decimalV2 This is not relevant any more. http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@88 PS1, Line 88: throw new InternalException(String.format( > Is it guaranteed that both types are decimal at this point? Actually no. Updated the error message text. http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impa
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 6: Code-Review+1 Looks good to me. David, do you want to give the final +2? -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 06 Apr 2018 05:17:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/9927/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9927/5/shell/impala_shell.py@1150 PS5, Line 1150: # args == current_db means -d option was passed but the "use [db]" operation failed. This comments needs to be updated. Move this comment under elif. -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 06 Apr 2018 00:21:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@781 PS4, Line 781: db=self.current_db if self.current_db else ImpalaShell.DEFAULT_DB) At this point we don't know if current_db exists or not. I think we should always set db to ImpalaShell.DEFAULT_DB here. It will get updated later. http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@1148 PS4, Line 1148: args Why not args.strip() here? http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@1156 PS4, Line 1156: if args.strip('`') == self.current_db: Make this an "elif". Also, prompt does not need to be updated because of my first comment. It's a good idea to set self.current_db = None here. -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Thu, 05 Apr 2018 01:33:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 3: I think it's ok to cherry pick this to 2.x -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Wed, 04 Apr 2018 23:44:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py@1151 PS3, Line 1151: strip('`') why do we strip() here, but not on line 781? http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py@1153 PS3, Line 1153: # args == current_db means -d option was passed but the database does not exist. It's confusing to me why args == current_db means that the db does not exist. Can you explain this a little better? What are the contents of args and current_db? -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Wed, 04 Apr 2018 23:43:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9930 Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE .. IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE In this patch we implement strict decimal type checking in the FE in various situations when DECIMAL_V2 is enabled. What is affected: - Union. If we union two decimals and it is not possible to come up with a decimal that will be able to contain all the digits, an error is thrown. For example, the union(decimal(20, 10), decimal(20, 20)) returns decimal(30, 20). However, for union(decimal(38, 0), decimal(38, 38)) the ideal return type would be decimal(76,38), but this is too large, so an error is thrown. - Insert. If we are inserting a decimal value into a column where we are not guaranteed that all digits will fit, an error is thrown. For example, inserting a decimal(38,0) value into a decimal(38,38) column. - Functions such as coalesce(). If we are unable to determine the output type that guarantees that all digits will fit from all the arguments, an error is thrown. For example, coalesce(decimal(38,38), decimal(38,0)) will throw an error. - Hash Join. When joining on two decimals, if a type cannot be determined that both columns can be cast to, we throw an error. For example, join on decimal(38,0) and decimal(38,38) will result in an error. To avoid these errors, you need to use CAST() on some of the decimals. In this patch we also change the output decimal calculation of decimal round, truncate and related functions. If these functions are a no-op, the resulting decimal type is the same as the input type. Testing: - Ran a core build and all tests passed Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java M fe/src/main/java/org/apache/impala/analysis/RangePartition.java M fe/src/main/java/org/apache/impala/analysis/StatementBase.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java M fe/src/main/java/org/apache/impala/catalog/Function.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test M testdata/workloads/functional-query/queries/QueryTest/decimal.test A testdata/workloads/functional-query/queries/QueryTest/insert_decimal.test M tests/query_test/test_insert.py 32 files changed, 461 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/1 -- To view, visit http://gerrit.cloudera.org:8080/9930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568 Gerrit-Change-Number: 9930 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9927 ) Change subject: IMPALA-6805: Show current database in Impala shell prompt .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py@777 PS2, Line 777: [%s:%s] %s I would prefer to change this to use the format() function. Something like "[{host}:{port}] {db}> ".format(host=self.impalad[0], port=self.impalad[1], db =... http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py@1147 PS2, Line 1147: [%s:%s] %s> Since this is used several times, this template should be a constant declared somewhere around line 137. http://gerrit.cloudera.org:8080/#/c/9927/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/9927/2/tests/shell/test_shell_interactive.py@62 PS2, Line 62: %s>" % db it's better to use format() here too. -- To view, visit http://gerrit.cloudera.org:8080/9927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc Gerrit-Change-Number: 9927 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Wed, 04 Apr 2018 22:03:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6791: distcc server setup script
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9901 ) Change subject: IMPALA-6791: distcc server setup script .. Patch Set 7: Code-Review+2 lgtm -- To view, visit http://gerrit.cloudera.org:8080/9901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb Gerrit-Change-Number: 9901 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Apr 2018 23:31:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6791: distcc server setup script
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9901 ) Change subject: IMPALA-6791: distcc server setup script .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh File bin/distcc/distcc_server_setup.sh: http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh@62 PS6, Line 62: a nit: 6 and 7 http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh@81 PS6, Line 81: # Configure ccache for distccd user Do you think it's a good idea to turn these comments into echos? I saw that Phil Z has been doing this in some of our bash scripts. -- To view, visit http://gerrit.cloudera.org:8080/9901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb Gerrit-Change-Number: 9901 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Apr 2018 23:19:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6791: distcc server setup script
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9901 ) Change subject: IMPALA-6791: distcc server setup script .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh File bin/distcc/distcc_server_setup.sh: http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh@20 PS4, Line 20: Ubuntu Also CentOS http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh@60 PS4, Line 60: DISTCCD_USER=nobody Do we need to check the version of the OS here, like we do for Ubuntu? -- To view, visit http://gerrit.cloudera.org:8080/9901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb Gerrit-Change-Number: 9901 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Apr 2018 22:12:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 13: Code-Review+1 (2 comments) I'm happy with this change. Alex, can you take a look please? http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5331 PS13, Line 5331: TestValue("width_bucket(1, 19.77, 999.9" We should test the two conditions that can lead to requiring int256_t separately, which is what we are trying to do here. Are we sure that one condition is true and the other is false in these two tests? http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5333 PS13, Line 5333: Maybe add a few more cases where int256_t is used? I would add these tests: - smallest values where int256_t is needed. - largest values where int256_t is not needed - largest values where int256_t is needed and there is no overflow - smallest values where int256_t is needed and there should be an overflow -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 13 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Mon, 02 Apr 2018 23:41:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 12: (4 comments) This change is getting close. Overall I'm pretty happy with it. http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@502 PS12, Line 502: Nit: This empty line is in the wrong place, it should be above bool overflow = false http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@503 PS12, Line 503: Decimal16Value range_size = max_range.template Subtract(input_scale, min_range, Nit: long line http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@544 PS12, Line 544: if (UNLIKELY(BitUtil::CountLeadingZeros(abs(buckets.value())) + : BitUtil::CountLeadingZeros(abs(dist_from_min.value())) <= 128)) { : needs_int256 = true; : } : : // Check if scaling up range size overflows and if there is a need to store the : // intermediate results in int256_t to avoid the overflow : if (UNLIKELY(!needs_int256 && BitUtil::CountLeadingZeros(range_size.value()) + : detail::MinLeadingZerosAfterScaling(BitUtil::CountLeadingZeros( : range_size.value()), input_scale) <= 128)) { : needs_int256 = true; These two if statements should be combined into 1. It's weird to check !needs_int256 in the second one. Combine the two conditions with an OR. http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@582 PS12, Line 582: )+1 nit: put spaces around the + -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 12 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Sat, 31 Mar 2018 00:56:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6070: Expose using Docker to run tests faster.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9085 ) Change subject: IMPALA-6070: Expose using Docker to run tests faster. .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/9085/4/bin/bootstrap_system.sh File bin/bootstrap_system.sh: http://gerrit.cloudera.org:8080/#/c/9085/4/bin/bootstrap_system.sh@131 PS4, Line 131: JDK_VERSION=7 Is there a reason why we use JDK 7 on Ubuntu 14.04? This means that the current apache/master branch does not work after running this script because it requires JDK 8 (because of Hadoop 3). Maybe add a comment why we can't use JDK8 on 14.04? -- To view, visit http://gerrit.cloudera.org:8080/9085 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82052ef31979564968effef13a3c6af0d5c62767 Gerrit-Change-Number: 9085 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Fri, 30 Mar 2018 00:00:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9346 ) Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns .. IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns Before this patch, the output type of round() ceil() floor() trunc() was not always the same as the input type. It was also inconsistent in general. For example, round(double) returned an integer, but round(double, int) returned a double. After looking at other database systems, we decided that the guideline should be that the output type should be the same as the input type. In this patch, we change the behavior of the previously mentioned functions so that if a double is given then a double is returned. We also modify the rounding behavior to always round away from zero. Before, we were rounding towards positive infinity in some cases. Testinging: - Updated tests - Ran an exhaustive build which passed. Cherry-picks: not for 2.x Change-Id: I77541678012edab70b182378b11ca8753be53f97 --- M be/src/exprs/expr-test.cc M be/src/exprs/math-functions-ir.cc M be/src/exprs/math-functions.h M be/src/exprs/scalar-fn-call.cc M common/function-registry/impala_functions.py M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test M testdata/workloads/functional-query/queries/QueryTest/exprs.test 7 files changed, 110 insertions(+), 68 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/4 -- To view, visit http://gerrit.cloudera.org:8080/9346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97 Gerrit-Change-Number: 9346 Gerrit-PatchSet: 4 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 ) Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc@152 PS2, Line 152: // TODO: should we support non-constant seed? > I think this will DCHECK if a negative scale is passed oops, fixed. http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266 PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'], > I think this is a question for Greg. It's mostly about whether this functio Agreed. -- To view, visit http://gerrit.cloudera.org:8080/9346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97 Gerrit-Change-Number: 9346 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 23 Mar 2018 22:37:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9768 ) Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x .. Patch Set 2: Code-Review+2 (1 comment) Forwarding the +2 http://gerrit.cloudera.org:8080/#/c/9768/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9768/1//COMMIT_MSG@9 PS1, Line 9: We enabled Decimal V2 by default on master (but not on the 2.x branch) > nit: typo Done -- To view, visit http://gerrit.cloudera.org:8080/9768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b Gerrit-Change-Number: 9768 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 23 Mar 2018 00:31:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9768 to look at the new patch set (#2). Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x .. IMPALA-6622: Backport parts of IMPALA-4924 to 2.x We enabled Decimal V2 by default on master (but not on the 2.x branch) in IMPALA-4924. There were some other code changes that are not specific to enableing Decimal V2 that are causing merge conflicts. In this patch, we backport those changes to reduce the chance of conflicts. Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b --- M be/src/exprs/expr-test.cc M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M testdata/workloads/functional-query/queries/QueryTest/exprs.test M tests/hs2/test_hs2.py M tests/query_test/test_aggregation.py M tests/query_test/test_decimal_casting.py 6 files changed, 127 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/9768/2 -- To view, visit http://gerrit.cloudera.org:8080/9768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b Gerrit-Change-Number: 9768 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 ) Change subject: IMPALA-4848: Add WIDTH_BUCKET() function .. Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@455 PS10, Line 455: a > Done This wasn't fixed. http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@505 PS10, Line 505: f > If expr < min_range, we return 0 Suppose min_range = -... and max_range=..., so the subtraction overflows. This function would return only 3 possible values then. If value=min_range-1, then we return 0. If value = max_range+1, then we return num_buckets. Otherwise, we return null. This seems like odd behavior to me. I think we should always return null if there is an overflow when computing range_size. What do you think? (btw, if we're going to do this, this needs to be moved up to the top of the function) http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@502 PS11, Line 502: Decimal16Value range_size; : range_size = nit: why not combine the two lines? http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@524 PS11, Line 524: if (int total_leading_zeros = BitUtil::CountLeadingZeros(abs(buckets.value())) + Why do we need to declare total_leading_zeros here? Also, make this UNLIKELY http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@529 PS11, Line 529: c nit: capital letter http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@531 PS11, Line 531: if (!needs_int256 && BitUtil::CountLeadingZeros(range_size.value()) + You should use MinLeadingZerosAfterScaling() here. Also, make this UNLIKELY http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h@115 PS10, Line 115: static BigIntVal WidthBucket(FunctionContext* ctx, const DecimalVal& expr, > I made this private. Not sure, any pointers where I can move this function I think the place you moved it to is ok. -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 11 Gerrit-Owner: anujphadke Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 23 Mar 2018 00:27:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9768 Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x .. IMPALA-6622: Backport parts of IMPALA-4924 to 2.x We enabled Deciaml V2 by default on master (but not on the 2.x branch) in IMPALA-4924. There were some other code changes that are not specific to enableing Decimal V2 that are causing merge conflicts. In this patch, we backport those changes to reduce the change of conflicts. Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b --- M be/src/exprs/expr-test.cc M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M testdata/workloads/functional-query/queries/QueryTest/exprs.test M tests/hs2/test_hs2.py M tests/query_test/test_aggregation.py M tests/query_test/test_decimal_casting.py 6 files changed, 127 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/9768/1 -- To view, visit http://gerrit.cloudera.org:8080/9768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: newchange Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b Gerrit-Change-Number: 9768 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9683 ) Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG@11 PS1, Line 11: nd support Nit: "and enables support of all hash algorithms" -- To view, visit http://gerrit.cloudera.org:8080/9683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b Gerrit-Change-Number: 9683 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Thu, 15 Mar 2018 23:50:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 ) Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns .. Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120 PS1, Line 120: return DoubleVal(trunc( > pow(10.0, scale.val) is likely expensive to compute and it isn't clear that Done. According to my benchmark, this implementation is much faster (over 10x) than the old one (for cases where scale is less than 20). http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120 PS1, Line 120: return DoubleVal(trunc( > Do we have a test that shows the better behavior for trunc() versus the pre Yes, Alex, there is a test in expr-test.cc on line 5465 http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121 PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val)); > Can you undo the code movement here so that the change is more clear? Done http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121 PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val)); > Should we also check for overflows here , set FunctionContext with an error Anuj, the problem here is with double arithmetic. In general, it's not possible to represent a double number precisely. Which is why it is weird to do rounding and specify the number of digits to round to in decimal. I don't think that anything can be done about this. This is exactly the reason why people should be using the decimal type. http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80 PS1, Line 80: static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, const BigIntVal&); > Why the ordering change? Also, why allow BigIntVal range here, isn't that Fixed the ordering. It makes sense to use bigintval because if someone passes in a smallint, it can be implicitly cast to bigint, but not the other way around. So bigint is kind of like an integer wildcard. Which overflow are you talking about? http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266 PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'], > Should we also have a DECIMAL version? Do you think we would need to have a sign() function for double, float, int, bigint, decimal, etc then? http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280 PS1, Line 280: [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Ceil'], > Don't we also need FLOAT versions of these if our goal is to return the sam I agree, this should be addressed in a separate patch. -- To view, visit http://gerrit.cloudera.org:8080/9346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97 Gerrit-Change-Number: 9346 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 15 Mar 2018 21:51:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9346 ) Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns .. IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns Before this patch, the output type of round() ceil() floor() trunc() was not always the same as the input type. It was also inconsistent in general. For example, round(double) returned an integer, but round(double, int) returned a double. After looking at other database systems, we decided that the guideline should be that the output type should be the same as the input type. In this patch, we change the behavior of the previously mentioned functions so that if a double is given then a double is returned. We also modify the rounding behavior to always round away from zero. Before, we were rounding towards positive infinity in some cases. Testinging: - Updated tests - Ran an exhaustive build which passed. Cherry-picks: not for 2.x Change-Id: I77541678012edab70b182378b11ca8753be53f97 --- M be/src/exprs/expr-test.cc M be/src/exprs/math-functions-ir.cc M be/src/exprs/math-functions.h M be/src/exprs/scalar-fn-call.cc M common/function-registry/impala_functions.py M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test M testdata/workloads/functional-query/queries/QueryTest/exprs.test 7 files changed, 107 insertions(+), 68 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/2 -- To view, visit http://gerrit.cloudera.org:8080/9346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97 Gerrit-Change-Number: 9346 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke
[Impala-ASF-CR](2.x) IMPALA-6405: Error when string to decimal cast overflows
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9530 ) Change subject: IMPALA-6405: Error when string to decimal cast overflows .. Patch Set 1: I started a dry run build: https://jenkins.impala.io/job/gerrit-verify-dryrun/2071/ I will submit the patch once the build passes. -- To view, visit http://gerrit.cloudera.org:8080/9530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db Gerrit-Change-Number: 9530 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Thu, 08 Mar 2018 22:34:19 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-6405: Error when string to decimal cast overflows
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9530 ) Change subject: IMPALA-6405: Error when string to decimal cast overflows .. Patch Set 1: Code-Review+2 Cherry-picked and resolved conflicts -- To view, visit http://gerrit.cloudera.org:8080/9530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db Gerrit-Change-Number: 9530 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Comment-Date: Thu, 08 Mar 2018 22:05:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 ) Change subject: IMPALA-6337: Fix infinite loop in Impala shell .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588 PS9, Line 588: for index in xrange(0, len(parsed_statements)): > Isn't xrange is faster in Python 2.7 compared to enumerate? It probably is faster, but I don't think performance should be a concern in this case. If you want to keep xrange(), then you can get rid of the first argument. xrange(len(parsed_statements)) -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Mar 2018 00:24:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 ) Change subject: IMPALA-6337: Fix infinite loop in Impala shell .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601 PS9, Line 601: if not found_error: > Ok, but what will happen if the list looks like the following: I tried it, it looks like it throws away the middle "not error" statement. I think that's weird behavior. I tried it with this query in impala-shell: select 1; \; select 2; \; select 3; "Select 2" vanishes -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Mar 2018 00:16:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 ) Change subject: IMPALA-6337: Fix infinite loop in Impala shell .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588 PS9, Line 588: for index in xrange(0, len(parsed_statements)): > for index, statement in enumate(parsed_statements) enumerate* -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Mar 2018 23:45:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 ) Change subject: IMPALA-6337: Fix infinite loop in Impala shell .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588 PS9, Line 588: for index in xrange(0, len(parsed_statements)): for index, statement in enumate(parsed_statements) -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Mar 2018 23:44:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 ) Change subject: IMPALA-6337: Fix infinite loop in Impala shell .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601 PS9, Line 601: if not found_error: > When found_error is true at the very end (implied by having non empty joine Ok, but what will happen if the list looks like the following: not error, error, not error, error, not error Also, maybe add this to the test cases? -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Mar 2018 23:42:32 + Gerrit-HasComments: Yes