date:20170913

Alex Behm has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1331: checkDecimalReturnType("select 1 + 1.1",
> 1 -> interpreted as tinyint -> decimal(3, 0)
Ahh right, thanks for the refresher.


Line 1386: Type.DOUBLE, ScalarType.createDecimalType(2, 1));
> So I guess the current behaviour seems OK to me but I haven't thought deepl
Agree.


http://gerrit.cloudera.org:8080/#/c/7916/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1476: checkDecimalReturnType("select 
12345678901234567890123456789012345678",
Feels more appropriate to put these into TestNumericLiteralMinMaxValues() at 
the top of this file. There are already some existing tests that we should 
dedup with your new tests. Perhaps some of the existing tests there should also 
use checkDecimalReturnType()


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Alex Behm has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
> Ah, I saw this too late and the change got submitted in the meantime. I don
No worries, I think it's fine as is.


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
> We should fix this msg, not correct anymore, sorry
Ah, I saw this too late and the change got submitted in the meantime. I don't 
think it warrants a force push, let me know if you think otherwise. Feel free 
to -2 my changes if I miss something similar in the future. :)


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

Pranay Singh has uploaded a new patch set (#2).

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 14 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8056/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

2017-09-13 Thread Tianyi Wang (Code Review)

Tianyi Wang has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8063

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..

IMPALA-5416: Fix an impala-shell command recursion bug

Impala-shell crashes with 2 source commands on the same line and runs
a command multiple times if it shares the same line with a source
command. This patch fixes these bugs.

Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
---
M shell/impala_shell.py
1 file changed, 4 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/8063/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

Pranay Singh has uploaded a new patch set (#2).

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 14 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8056/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

Pranay Singh has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8056/1/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

Line 68
> Avoid unrelated whitespace diffs.
Thanks I was able to fix this issue


PS1, Line 96: fq_tbl_name = "functional_parquet" + "." + tbl_name
> I'm wary of creating tables in our default schemas. This won't get cleaned 
Shall I drop the table after running fuzz test ?


PS1, Line 98: create = ("create table {0} stored as parquet as select * 
from functional.alltypes"
:   .format(fq_tbl_name))
> I think we need to verify that the right options are being set when we crea
I introduced a check that compression_codec == none is only used here  line #93


-- 
To view, visit http://gerrit.cloudera.org:8080/8056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE

Pranay Singh has posted comments on this change.

Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7725/2/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:

PS2, Line 142:   case TUnit::DOUBLE_VALUE: {
 : double output = static_cast(value);
 : ss << std::setprecision(precision) << output << " ";
> I think TUnit::DOUBLE_VALUE is so similar to TUnit::UNIT that we should be 
I thought of removing TUnit::DOUBLE_VALUE from the code and replacing it with 
TUnit::UNIT but was perplexed by two cases;

a) TUnit::UNIT only obeys precision for larger values: It conditionally sets 
the precision for large values greater than 1000, so how should a caller 
display a smaller value in floating point representation restricted to two 
decimal places(or any determined by precision). Say this can be done by setting 
the precision argument but the second issue is more confounding.

b) Printing different variables of TUnit::UNIT type with different precision: 
Use of common function like RuntimeProfile::PrintChildCounters() to display 
variables of same type with different precision makes it hard to use 
TUnit::UNIT type, it appears that TUnit::DOUBLE is a way to Print some 
variables with smaller value to be restricted to 2 decimal places (or any 
precision) and the others to be displayed without any restriction.

Hence I thought that TUnit::DOUBLE_VALUE cannot be removed, so  made changes 
for it.


-- 
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5926: Avoid printing expensive stack when closing a session

Alex Behm has posted comments on this change.

Change subject: IMPALA-5926: Avoid printing expensive stack when closing a 
session
..


Patch Set 2: Code-Review+2

Remove space from commit msg. Carry +2

-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5926: Avoid printing expensive stack when closing a session

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8060

to look at the new patch set (#2).

Change subject: IMPALA-5926: Avoid printing expensive stack when closing a 
session
..

IMPALA-5926: Avoid printing expensive stack when closing a session

When conducting high concurrency tests for short running queries
 noticed that queries are spending lots of time in Unregister query.
Investigation showed that CloseSessionInternal calls
status("Session closed") which unnecessarily prints the stack
to the log which is expensive and not required, refer to IMPALA-5275.

The fix uses Expected(const std::string& error_msg) which doesn't
print the stack.

Table below summarizes speedup for highly selective scan query.

+---+--+-+-+
| Num users | Baseline Queries/minutes | Fix Queries/minutes | Speedup |
+---+--+-+-+
| 1 | 19   | 24  | 1.23x   |
| 2 | 41   | 48  | 1.17x   |
| 4 | 71   | 91  | 1.28x   |
| 8 | 96   | 161 | 1.67x   |
| 16| 117  | 226 | 1.92x   |
| 32| 140  | 266 | 1.90x   |
| 64| 174  | 269 | 1.54x   |
| 128   | 202  | 265 | 1.31x   |
+---+--+-+-+

Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
---
M be/src/service/impala-server.cc
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/8060/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5926 : Avoid printing expensive stack when closing a session

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5926 : Avoid printing expensive stack when closing a 
session
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

Alex Behm has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 7:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 49:  * x is [not] distinct from x -> false [true]
this is handled in your new rule


http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1642: AnalysisError("select nullif(1,2,3)", "default.nullif() 
unknown");
Weird. We should consider fixing this later by perhaps adding a dummy nullif() 
signature to the builtins db.

I think there's some general thinking to be done about these kind of converted 
exprs, e.g. how can we make them appear in "show functions".

But not for this patch. Can you file a JIRA for this improvement?


http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 48:   static class CountingRewriteRuleWrapper implements ExprRewriteRule {
static not needed


Line 58:   if (expr != ret) {
single line


Line 128:   // All specified rules must have fired at least once.
Explain why.


Line 312: RewritesOk("nullif(NULL, NULL)", rule, "NULL");
Did you check whether these new tests are not already addressed by 
expr-test.cc? For constant folding, it's generally preferable to add new tests 
in expr-test.cc to keep everything in one place.

The tests here usually deal with FE<->BE specific issues for constant folding, 
or with particularly tricky folding cases.


Line 579:   public void TestDistinctFromSimplificationRule() throws 
AnalysisException {
TestSimplifyDistinctFromRule()


Line 603:* it can be further simplified via DistinctFromSimplificatinoRule.
SimplifyDistinctFromRule


Line 619: // highlights that nullif gets transformed; the test 
infrastructure
I don't get the part about the test infrastructure.


-- 
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

2017-09-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

This change also removes PrintAsHex() which was broken and unused.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Reviewed-on: http://gerrit.cloudera.org:8080/8050
Reviewed-by: Lars Volker 
Tested-by: Impala Public Jenkins
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
3 files changed, 10 insertions(+), 12 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Lars Volker: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

2017-09-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5856: Fix outer join predicate assignment.

Alex Behm has uploaded a new patch set (#2).

Change subject: IMPALA-5856: Fix outer join predicate assignment.
..

IMPALA-5856: Fix outer join predicate assignment.

Fixes incorrect assignment of join predicates with
the following properties:
- from the On-clause of a left outer join
- only references the left-hand side tuples (not the
  right hand side tuple)
- references full-outer joined tuples; the full outer
  join appears on the left

Testing:
- a core/hdfs run passed
- added new regression test

Change-Id: I93db34d988cb66e00aa05d7dc161e0ca47042acb
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
2 files changed, 46 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8039/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8039
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I93db34d988cb66e00aa05d7dc161e0ca47042acb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis

[Impala-ASF-CR] IMPALA-5856: Fix outer join predicate assignment.

Alex Behm has posted comments on this change.

Change subject: IMPALA-5856: Fix outer join predicate assignment.
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8039/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS1, Line 1381: // Right-most full-outer join table ref that is equal to or to 
the left of the
  : // table ref materializing 'tids'.
> Trying to visualize this hurts my brain :) Not sure if it helps to understa
Agree, difficult to comprehend. Removed.


PS1, Line 1385: this
> What is "this" refer to? After reading this more carefully, I think I under
Rephrased. The comment is supposed to describe what 'ojRef' is - that's why 
it's above 'ojRef'.


PS1, Line 1398: globalState_.ojClauseByConjunct.get(e.getId());
> getOjRef(e)?
Done


http://gerrit.cloudera.org:8080/#/c/8039/1/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test:

PS1, Line 1020: from the On-clause of a left outer join
> Out of curiosity, did you check if we have the same issue if there is a lef
We have a test for that in L634. Please check if you think it's sufficient.

I don't think those cases are similar because if the FOJ comes after the LOJ, 
then only the FOJ On-clause predicates are considered to be full-outer-joined 
(and will be handled by the non-full-outer join assignment code path).


Line 1028:  and t1.bigint_col > 10 and t2.bigint_col > 30
> To make it more interesting, add a where clause with one predicate that ref
Good idea. Done.


-- 
To view, visit http://gerrit.cloudera.org:8080/8039
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I93db34d988cb66e00aa05d7dc161e0ca47042acb
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5905: build-all-flag-combinations addendum

2017-09-13 Thread Matthew Jacobs (Code Review)

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5905: build-all-flag-combinations addendum
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa97a981846a2397ecabb90b9039ba61c2c7af4e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5926 : Avoid printing expensive stack when closing a session

2017-09-13 Thread Mostafa Mokhtar (Code Review)

Mostafa Mokhtar has abandoned this change.

Change subject: IMPALA-5926 : Avoid printing expensive stack when closing a 
session
..


Abandoned

-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar 
Gerrit-Reviewer: Alex Behm

[Impala-ASF-CR] IMPALA-5905: build-all-flag-combinations addendum

2017-09-13 Thread Philip Zeyliger (Code Review)

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5905: build-all-flag-combinations addendum
..


Patch Set 1: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/8061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa97a981846a2397ecabb90b9039ba61c2c7af4e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

2017-09-13 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed
parquet
..

Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8056/1/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

Line 68
Avoid unrelated whitespace diffs.

One way of getting a graphical diff that can help with this is to use the tool
meld. For example:

git difftool -y -t meld

Where could be asf-gerrit/master or origin/master or whatnot.

PS1, Line 96: fq_tbl_name = "functional_parquet" + "." + tbl_name
I'm wary of creating tables in our default schemas. This won't get cleaned up,
and it is subtle behavior. If we can create the new table in the
unique_database that would be nice

PS1, Line 98: create = ("create table {0} stored as parquet as select *
from functional.alltypes"
: .format(fq_tbl_name))
I think we need to verify that the right options are being set when we create
this table. As I understand it, you need to specify the query option
compression_codec = none to create a parquet file without compression.

https://www.cloudera.com/documentation/enterprise/5-8-x/topics/impala_compression_codec.html

--
To view, visit http://gerrit.cloudera.org:8080/8056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5926 : Avoid printing expensive stack when closing a session

Alex Behm has posted comments on this change.

Change subject: IMPALA-5926 : Avoid printing expensive stack when closing a 
session
..


Patch Set 1: Code-Review+1

Amazing.

-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar 
Gerrit-Reviewer: Alex Behm 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5905: build-all-flag-combinations addendum

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8061

Change subject: IMPALA-5905: build-all-flag-combinations addendum
..

IMPALA-5905: build-all-flag-combinations addendum

Running the script under Jenkins revealed a couple of issues that I'd
missed

Change-Id: Iaa97a981846a2397ecabb90b9039ba61c2c7af4e
---
M bin/jenkins/build-all-flag-combinations.sh
1 file changed, 2 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/8061/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa97a981846a2397ecabb90b9039ba61c2c7af4e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 1:

(4 comments)

Nice to see this getting fixed.

http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/exec/filter-context.h
File be/src/exec/filter-context.h:

Line 124:   static Status CodegenInsert(LlvmCodeGen* codegen, ScalarExpr* 
filter_expr,
This is a bit tricky because it doesn't actually do exactly the same thing as 
Insert() - it doesn't check if the filter is NULL. That would be worth 
documenting, but I think the null check is actually necessary so the resolution 
is probably just to make it do the same thing as Insert().


http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 953:   if (num_filters == 0) {
Nit: it looks like the two branches are identical except for adding the 
NoInline. Might be clearer just to write:

  if (num_filters > 0) {
   insert_runtime_filters_fn->addFnAttr(llvm::Attribute::NoInline);
  }


Line 958:   if (filter_ctxs_[i].local_bloom_filter != NULL) {
Going forward, we want to avoid have this dependence between the codegen code 
and fragment instance-local state: see IMPALA-4080 - so that the same codegen'd 
code can be shared between instances of the same plan node. I.e. this should be 
a runtime branch.


http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/util/bloom-filter-ir.cc
File be/src/util/bloom-filter-ir.cc:

Line 32:   if (CpuInfo::IsSupported(CpuInfo::AVX2)) {
It would be nice to keep this in header file to avoid regressing the 
non-codegen'd path (although that likely won't be a big deal).

It would also be nice to avoid this AVX2 branch. Maybe we could just have 3 
versions - the original inlined one in the header and then two IR ones here for 
the AVX and non-AVX2 cases.


-- 
To view, visit http://gerrit.cloudera.org:8080/8029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5926 : Avoid printing expensive stack when closing a session

2017-09-13 Thread Mostafa Mokhtar (Code Review)

Mostafa Mokhtar has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8060

Change subject: IMPALA-5926 : Avoid printing expensive stack when closing a 
session
..

IMPALA-5926 : Avoid printing expensive stack when closing a session

When conducting high concurrency tests for short running queries
 noticed that queries are spending lots of time in Unregister query.
Investigation showed that CloseSessionInternal calls
status("Session closed") which unnecessarily prints the stack
to the log which is expensive and not required, refer to IMPALA-5275.

The fix uses Expected(const std::string& error_msg) which doesn't
print the stack.

Table below summarizes speedup for highly selective scan query.

+---+--+-+-+
| Num users | Baseline Queries/minutes | Fix Queries/minutes | Speedup |
+---+--+-+-+
| 1 | 19   | 24  | 1.23x   |
| 2 | 41   | 48  | 1.17x   |
| 4 | 71   | 91  | 1.28x   |
| 8 | 96   | 161 | 1.67x   |
| 16| 117  | 226 | 1.92x   |
| 32| 140  | 266 | 1.90x   |
| 64| 174  | 269 | 1.54x   |
| 128   | 202  | 265 | 1.31x   |
+---+--+-+-+

Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
---
M be/src/service/impala-server.cc
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/8060/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8060
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9d0ca485dd17bca758d916040745288c1a20c69f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Mostafa Mokhtar

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8035/2/common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 19: //YARNUTIL: MODIFIED
> Yeah, based on the CDH source tarball, e.g.
In case it benefits anyone else I was able to look at the diffs by downloading 
the tarball and using the following bash:

  for f in $(find 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ 
-name *.java) ; do vimdiff $(echo $f | sed 
's/common\/yarn-extras\//\/home\/tarmstrong\/hadoop-2.6.0-cdh5.12.0\/src\/hadoop-yarn-project\/hadoop-yarn\/hadoop-yarn-server\/hadoop-yarn-server-resourcemanager\//')
 $f; done


-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE

2017-09-13 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7725/2/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:

PS2, Line 142:   case TUnit::DOUBLE_VALUE: {
 : double output = static_cast(value);
 : ss << std::setprecision(precision) << output << " ";
I think TUnit::DOUBLE_VALUE is so similar to TUnit::UNIT that we should be 
looking to remove uses of DOUBLE_VALUE and allow UNIT to specify a precision.


-- 
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Alex Behm has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..


Patch Set 8:

Our other approach for these small rewrites can be seen in 
FunctionCallExpr#createExpr(). Basically we transform directly in the parser. 
An example of that approach is NVL2().

I still think it makes sense to unify with IsNUllPredicate in the grand scheme 
of things.

-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: Revert "Update download and signature links for 2.10.0 release."
..


Patch Set 1: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has submitted this change and it was merged.

Change subject: Revert "Update download and signature links for 2.10.0 release."
..


Revert "Update download and signature links for 2.10.0 release."

This reverts commit ff9d269a20ed1e452fe3a2f5450e71fcdcc4f30e.

Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Reviewed-on: http://gerrit.cloudera.org:8080/8057
Reviewed-by: Jim Apple 
Tested-by: Bharath Vissapragada 
---
M downloads.html
1 file changed, 6 insertions(+), 16 deletions(-)

Approvals:
  Bharath Vissapragada: Verified
  Jim Apple: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple

[Impala-ASF-CR] IMPALA-5250: Unify decompressor output length semantics

2017-09-13 Thread Tianyi Wang (Code Review)

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5250: Unify decompressor output_length semantics
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8030/1//COMMIT_MSG
Commit Message:

Line 26: instead of the "fast" version, for the latter is insecure with 
corrupted
> Not sure how useful compression-test.cc is. I was thinking more along the l
Running single_node_perf_run.py locally shows that overall performance is 1.78% 
slower after this change. Given LZ4 is not used in parquet, we may leave LZ4 
inconsistent with others for now.


-- 
To view, visit http://gerrit.cloudera.org:8080/8030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

2017-09-13 Thread Vuk Ercegovac (Code Review)

Vuk Ercegovac has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..


Patch Set 8:

thanks for the suggestion!

I got the sense that the rewrite approach might be a bit tricky when I saw 
special casing for between in the analyzer. Would like to see if such small 
rewrites can be easier however. I'll try the other approach in a separate 
change.

-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5844: use a MemPool for expr local allocations

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5844: use a MemPool for expr local allocations
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exprs/scalar-expr-evaluator.h
File be/src/exprs/scalar-expr-evaluator.h:

Line 81:   MemPool* expr_mem_pool, MemPool* local_mem_pool, 
ScalarExprEvaluator** eval) WARN_UNUSED_RESULT;
Long line.


PS6, Line 221: local
runtime-managed (a.k.a. "local")


-- 
To view, visit http://gerrit.cloudera.org:8080/8025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5985a7d45b61
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5844: use a MemPool for expr local allocations

Tim Armstrong has uploaded a new patch set (#6).

Change subject: IMPALA-5844: use a MemPool for expr local allocations
..

IMPALA-5844: use a MemPool for expr local allocations

Local allocations in expressions have the following properties:
* They are usually small allocations
* They can be made frequently (e.g. every function call)
* They are owned and managed by the Impala runtime
* They are freed in bulk at various points in query execution.

A MemPool (i.e. bump allocator) is the right mechanism to manage
allocations with the above properties. Before this patch
FunctionContext's used a FreePool + vector of allocations to emulate the
above behaviour. This patch switches to using a MemPool to bring these
allocations in line with the rest of the codebase.

The steps required to do this conversion.
* Use a MemPool for FunctionContext local allocations.
* Identify appropriate MemPools for all of the local allocations from
  function contexts so that the memory lifetime is correct.
* Various cleanup and documentation of existing MemPools.
* Replaces calls to FreeLocalAllocations() with calls to
  MemPool::Clear()

More involved surgery was required in a few places:
* Made the Sorter own its comparator, exprs and MemPool.
* Remove FunctionContext::ReallocateLocal() and just have
  StringFunctions::Replace() do the doubling itself to avoid
  the need for a special interface. Worst-case this doubles
  the memory requirements for Replace() since n / 2 + n / 4
  + n / 8 +  bytes of memory could be wasted instead of recycled
  for an n-byte output string.
* Provide a way redirect agg fn Serialize()/Finalize() allocations
  to come directly from the output RowBatch's MemPool. This is
  also potentially applicable to other places where we currently
  copy out strings from local allocations, e.g.
  AnalyticEvalNode::AddResultTuple() and Tuple::MaterializeExprs().
* --stress_free_pool_alloc was changed to instead intercept at the
  FunctionContext layer so that it retains the old behaviour even
  though allocations do not all come from FreePools.

Testing:
* ran exhaustive and ASAN

Change-Id: I4ba5a7542ed90a49a4b5586c040b5985a7d45b61
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/common/global-flags.cc
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/filter-context.cc
M be/src/exec/filter-context.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/kudu-scanner.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/unnest-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn-evaluator.h
M be/src/exprs/case-expr.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-evaluator.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/string-functions-ir.cc
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
D be/src/runtime/free-pool.cc
M be/src/runtime/free-pool.h
M be/src/runtime/sorted-run-merger.cc
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf-test-harness.cc
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
M tests/custom_cluster/test_alloc_fail.py
72 files changed, 621 insertions(+), 741 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8025/6
-- 
To view, visit http://gerrit.cloudera.org:8080/8025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5985a7d45b61
Gerrit-PatchSet: 6
Gerrit-Project:

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

2017-09-13 Thread Philip Zeyliger (Code Review)

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 7:

I should also mention that previous commit messages referred to "ifnull" where 
they should have referred to "nullif". It's not surprising that these are 
confusing, but it's worth noting that we didn't catch that in several rounds of 
reading.

-- 
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

2017-09-13 Thread Philip Zeyliger (Code Review)

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7829

to look at the new patch set (#7).

Change subject: IMPALA-5211: Simplifying nullif conditional.
..

IMPALA-5211: Simplifying nullif conditional.

This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.

As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.

It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)

Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
  distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.

Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.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/ParserTest.java
19 files changed, 230 insertions(+), 122 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Alex Behm has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..


Patch Set 8:

Vuk, I have concerns regarding the high-level approach. From a bird's eye view 
this patch adds a lot of new code and complexity for a simple new function.

I think the simplest (and preferable) implementation would look like this:
1. Unify the new expr with the existing IsNullPredicate; the new expr provides 
a super set of functionality
2. Have a proper BE implementation; the function should have 3 arguments: one 
is the input expr, another is a boolean indicating negation and the third is an 
int indicating the kind of check we are doing (checking for 
isnull/isfalse/istrue). That way we can codegen away the runtime overhead of 
checking those cases.

The current solution takes the approach of BetweenPredicate which is tricky and 
error prone. For example, in your patch you forget to add the new rewrite rule 
into the HdfsPartitionPruner. Overall it's tricky to find all those places, and 
In think that eventually we may want to change how BetweenPredicate works as 
well.

What do you think? Happy to discuss.

-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

2017-09-13 Thread Matthew Jacobs (Code Review)

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8035/2//COMMIT_MSG
Commit Message:

Line 23: 'common/yarn-extras', taken from Hadoop 2.6.0.
> Is the original source code that you cribbed this from available publicly s
Done


http://gerrit.cloudera.org:8080/#/c/8035/2/common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 19: //YARNUTIL: MODIFIED
> Is there an easy way I can see the diff? If you point me to the hadoop sour
Yeah, based on the CDH source tarball, e.g.
https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html#cm_vd_cdh_package_tarball_512

I'll update the commit message.


http://gerrit.cloudera.org:8080/#/c/8035/2/fe/pom.xml
File fe/pom.xml:

Line 111:   system
> Is systemPath necessary? We seem to be able to resolve data-source-api abov
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

2017-09-13 Thread Matthew Jacobs (Code Review)

Matthew Jacobs has uploaded a new patch set (#3).

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..

IMPALA-5920: Remove admission control dependency on YARN RM jar

Impala's admission controller relies on the YARN
fair-scheduler.xml for configuration. That configuration is
loaded using YARN directly (ie. as a library by the
frontend). In Hadoop 3, a number of changes were made to the
YARN resourcemanager which break Impala. While we eventually
want to rethink the admission control configuration
(IMPALA-4159), in the meantime we at least should avoid
using unsupported YARN APIs.

This patch removes the fe dependency on the YARN artifact
'hadoop-yarn-server-resourcemanager' which contains private
APIs and isn't meant to be used as a library.

A subset of the required code has been added in
'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g.
see [1].

The goal the 'yarn-extras' is to make Impala's handling of
the AC configuration self-sufficient, i.e. it shouldn't
matter what version of Hadoop exists. As-is, this was tested
and found to work when Hadoop 2.6 is installed. Because
the yarn-extras/pom.xml still references hadoop-common,
hadoop-yarn-common, and hadoop-yarn-api, additional testing
will be required to ensure Impala using yarn-extras works
when installed along side Hadoop 3.

That testing for Hadoop 3 will be done later. Future changes
will make any other changes required for existing code to
work when Hadoop 3 is installed.

Testing:
* Ran existing tests on master.

1: 
https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html

Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
---
M CMakeLists.txt
A common/yarn-extras/CMakeLists.txt
A common/yarn-extras/README.txt
A common/yarn-extras/pom.xml
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/resource/ResourceWeights.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java
M fe/CMakeLists.txt
M fe/pom.xml
M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
17 files changed, 1,802 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Alex Behm has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
We should fix this msg, not correct anymore, sorry


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

2017-09-13 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: Revert "Update download and signature links for 2.10.0 release."
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3: Code-Review+2

Carrying Alex's +2.

-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 139: VLOG_QUERY << status;
> VLOG_QUERY << "Cancelling and closing child query. Failed to get query id: 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

2017-09-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1219/

-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Hello Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8050

to look at the new patch set (#3).

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..

IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

This change also removes PrintAsHex() which was broken and unused.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
3 files changed, 10 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8050/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Alex Behm has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 139: VLOG_QUERY << status;
VLOG_QUERY << "Cancelling and closing child query. Failed to get query id: " << 
status;


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

2017-09-13 Thread Vuk Ercegovac (Code Review)

Vuk Ercegovac has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS7, Line 333: BooTestPre
> BoolTestPredicates?
Done


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java:

PS7, Line 28:  IS [NOT] .
> Replace with the one in L31.
Done


PS7, Line 30: ool, compatible with bool or null
> Remove, it's kind of redundant.
Done


http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

PS6, Line 591: for (String r
> Sorry, I don't think I understand this comment. What do you mean by analysi
ignore that comment. tests are now included here.


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

PS7, Line 217: "(bool_col is null and string_col = '10' and int_col < 10)", 
rule,
 : "int_col < 10 AND bool_col IS NULL AND " +
 : "((bigint_col < 10) OR (string_col = '10'))");
 : // Negated common conjunct: !(int_col=5 or tinyint_col > 9)
 : RewritesOk(
 : "(!(int_col=5 or tinyint_col > 9) and double_col = 7) or 
" +
 : "(!(int_col=5 or tinyint_col > 9) and double_col = 8
> Hm, it's not clear to me why this should be here and not as an Analysis tes
agreed, I understood incorrectly that analysis tests also run rewrites and that 
these rewrite tests are just for testing the result of rewrites, not including 
the subsequent analysis of the rewritten expression. 
removed these tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

2017-09-13 Thread Vuk Ercegovac (Code Review)

Vuk Ercegovac has uploaded a new patch set (#8).

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..

IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Adds a new expression to represent the following boolean predicate:
 IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.

Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions

Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.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/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 462 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/8
-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/1/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 131:   const string& guid = hs2_handle_.operationId.guid;
> Why not use ImpalaServer::THandleIdentifierToTUniqueId() to convert to a TU
Thanks, I didn't know what one.


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Lars Volker has uploaded a new patch set (#2).

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..

IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

This change also removes PrintAsHex() which was broken and unused.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
3 files changed, 9 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8050/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: Revert "Update download and signature links for 2.10.0 release."
..


Patch Set 1:

ASF policy requires that we need to wait for at least 24 hours before updating 
the release artefacts' links so that all the mirrors can catch up. Reverting 
this website change till then.

-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8057

Change subject: Revert "Update download and signature links for 2.10.0 release."
..

Revert "Update download and signature links for 2.10.0 release."

This reverts commit ff9d269a20ed1e452fe3a2f5450e71fcdcc4f30e.

Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
---
M downloads.html
1 file changed, 6 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/8057/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 3: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has submitted this change and it was merged.

Change subject: Update download and signature links for 2.10.0 release.
..


Update download and signature links for 2.10.0 release.

Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Reviewed-on: http://gerrit.cloudera.org:8080/8052
Reviewed-by: Jim Apple 
Tested-by: Bharath Vissapragada 
---
M downloads.html
1 file changed, 16 insertions(+), 6 deletions(-)

Approvals:
  Bharath Vissapragada: Verified
  Jim Apple: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker

[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE

Pranay Singh has uploaded a new patch set (#2).

Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
..

IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE

Fix: Current code uses reinterpret_cast for printing
 TCounterType::DOUBLE_VALUE, which is unsafe. It also uses a
 fixed precision, 2 for printing which may not be desirable.

 This change removes the usage of reinterpret_cast and provides
 an option to specify precision as an argument type to
 Print TCounterType::DOUBLE_VALUE.

Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
---
M be/src/util/pretty-printer-test.cc
M be/src/util/pretty-printer.h
M be/src/util/runtime-profile.cc
3 files changed, 20 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/7725/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/2/downloads.html
File downloads.html:

PS2, Line 163: 1
> 512
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

Pranay Singh has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8056

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 13 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8056/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Hello Lars Volker,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8052

to look at the new patch set (#3).

Change subject: Update download and signature links for 2.10.0 release.
..

Update download and signature links for 2.10.0 release.

Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
---
M downloads.html
1 file changed, 16 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/8052/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/2/downloads.html
File downloads.html:

PS2, Line 163: 1
512


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/1/downloads.html
File downloads.html:

Line 162: 
"https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incubating-2.10.0.tar.gz.sha;>
> 404. https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incu
Ouch, missed that. Thanks.


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Hello Lars Volker,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8052

to look at the new patch set (#2).

Change subject: Update download and signature links for 2.10.0 release.
..

Update download and signature links for 2.10.0 release.

Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
---
M downloads.html
1 file changed, 15 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/8052/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

2017-09-13 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS7, Line 333: Bool tests
BoolTestPredicates?


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java:

PS7, Line 28:  IS [NOT] 
Replace with the one in L31.


PS7, Line 30:  is one of (TRUE | FALSE | UNKNOWN).
Remove, it's kind of redundant.


http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

PS6, Line 591: 
> added negative tests and coverage to rewrites and end-to-end. the analysis 
Sorry, I don't think I understand this comment. What do you mean by analysis 
for the expr does not do much? Aren't the negative test cases captured by 
AnalysisError()? I think rewrite tests should handle the rewrite logic but 
everything else (analysis related) should be tested here. Maybe I am missing 
something.


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

PS7, Line 217: // Incorrect type for LHS.
 : AnalyzedRewritesError("'foo' is true", rule, "'foo' = TRUE 
AND 'foo' IS NOT NULL",
 : "operands of type STRING and BOOLEAN are not comparable: 
'foo' = TRUE");
 : // No subqueries allowed.
 : RewritesError(
 : "(select max(tinyint_col) from functional.alltypessmall) 
is true", rule,
 : "Subqueries are not supported in the select list.");
Hm, it's not clear to me why this should be here and not as an Analysis test.


-- 
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

Alex Behm has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/1/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 131:   const string& guid = hs2_handle_.operationId.guid;
Why not use ImpalaServer::THandleIdentifierToTUniqueId() to convert to a 
TUniqueId?

You can take a look at ExecuteStatement() in impala-hs2-server.cc that converts 
out internal query id to the HS2 handle guid


-- 
To view, visit http://gerrit.cloudera.org:8080/8050
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1386: Type.DOUBLE, ScalarType.createDecimalType(2, 1));
> That's the current behaviour and it does treat literals consistently with o
So I guess the current behaviour seems OK to me but I haven't thought deeply. 
Maybe it's worth revisiting but that's a separate discussion from handling of 
literals.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 513:* Converts numeric literal in the expr tree rooted at this expr to 
return floating
> literals
Done


Line 516:* Decimal has a higher processing cost than floating point and we 
should not pay
> Please add subsections "DECIMAL_V1" and "DECIMAL_V2" to explain the rationa
Done. I didn't expend many characters on the DECIMAL_V2 section but I think 
it's sufficient to explain the existence of the two code paths (since the 
comment will go away when DECIMAL_V1 goes away).


http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1306:* Test expressions that resolve to different types depend on the 
DECIMAL_V2 setting.
> Test expressions that return different decimal types depending on the DECIM
Reworded slightly since some don't return decimal types, just involve decimal 
somehow.


Line 1319:* Test expressions that resolve to the same type with either 
DECIMAL_V2 value.
> resolve to -> return
Done


Line 1331: Type.DOUBLE, ScalarType.createDecimalType(5, 1));
> Why is the result not DECIMAL(2,1)?
1 -> interpreted as tinyint -> decimal(3, 0)
1.1 -> interpreted as decimal(2,1)

Then adding decimal(3, 0) and decimal(2, 1) generally results in decimal(5, 1).

This makes sense to me. It's not perfect but it's consistent.

To get the tighter bound on the decimal type you'd need to interpret literals 
without decimal points as decimal literals (or do something even more 
complicated in the type system).


Line 1342: // DECIMAL_V2: floating point + decimal expr = decimal
> This does not seem to explain what happens for "floating point + numeric li
I don't think the original comment conveyed my intent well. 
The comment is intended to describe the general rules that apply for DECIMAL_V1 
and DECIMAL_V2. DECIMAL_V1's rules treat all numeric literals the same way in 
this context, whereas DECIMAL_V2's rules treat all decimal exprs the same way.

The R.H.S. expressions in these cases are decimal literals, so they are in the 
intersection of decimal exprs and numeric literals.


Line 1386: // Multiplying a floating point type with any other type always 
results in a double.
> Why? Seems inconsistent.
That's the current behaviour and it does treat literals consistently with other 
exprs. I think the idea is that multiplication is more likely to overflow 
fixed-point values. The code in getArithmeticResultType() is:

  // For multiplications involving at least one floating point type we cast 
decimal to
  // double in order to prevent decimals from overflowing.
  if (op == ArithmeticExpr.Operator.MULTIPLY &&
  (t1.isFloatingPointType() || t2.isFloatingPointType())) {
return Type.DOUBLE;
  }


Line 1409: checkDecimalReturnType("select round(1.2345, 2) * pow(10, 10)", 
Type.DOUBLE);
> add same test with "+"
Done. Also updated the comment because it was a little unclear that it was 
testing a very specific pattern instead of general expressions with decimal + 
double.


Line 1426: // Test behavior of compound expressions with a single column 
and many literals.
> column -> slot ref
Done


Line 1465: checkDecimalReturnType("select 1.123e-2", 
ScalarType.createDecimalType(5, 5));
> What about literals that don't fit into a decimal?
Added some more tests along those lines.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

Hello Greg Rahn,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7916

to look at the new patch set (#6).

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..

IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion

This changes the behaviour of applying an arithmetic operator to
constant DECIMAL and non-DECIMAL arguments. In DECIMAL_V1, this
caused an implicit conversion to floating point, which caused
users a lot of confusion in some cases. In DECIMAL_V2 the typing
rules are simplified: constant decimals are treated the same as any
other decimals.

Testing:
Added some expression tests for different arithmetic operators
and binary predicates (the two Expr subclasses that call
convertNumericLiteralsFromDecimal()).

Extended analyzer tests to test DECIMAL_V2 behaviour. Added many
additional test for various combinations of literals and non-literals to
get better coverage of existing and new behaviour.

Ran core tests.

Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 258 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7916/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/1/downloads.html
File downloads.html:

Line 162: 
"https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incubating-2.10.0.tar.gz.sha;>
404. 
https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incubating-2.10.0.tar.gz.sha512
 ?


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

Lars Volker has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

2017-09-13 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8052

Change subject: Update download and signature links for 2.10.0 release.
..

Update download and signature links for 2.10.0 release.

Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
---
M downloads.html
1 file changed, 15 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/8052/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

Lars Volker has posted comments on this change.

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 1:

As discussed via email, let's add tests after IMPALA-5664 has been fixed.

-- 
To view, visit http://gerrit.cloudera.org:8080/8051
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

2017-09-13 Thread Csaba Ringhofer (Code Review)

Csaba Ringhofer has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8051

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..

IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

The timestamp conversion from negative fractional Decimal types
(interpreted as unix timestamp) was wrong - the whole part
was rounded toward zero, and fractional part was being added
instead of being subtracted.

Example for the wrong behaivour:
+-+
| cast(cast(-0.1 as decimal(20,10)) as timestamp) |
+-+
| 1970-01-01 00:00:00.1   |
+-+
while casting to double works correctly:
+-+
| cast(cast(-0.1 as double) as timestamp) |
+-+
| 1969-12-31 23:59:59.9   |
+-+

Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
1 file changed, 3 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8051
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer

[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5905: add script for all-build-options job
..


Patch Set 3: Code-Review+2 Verified+1

Carry +2. Verified manually by running locally (no point in running the merge 
job since it doesn't execute this script yet).

-- 
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job

Tim Armstrong has submitted this change and it was merged.

Change subject: IMPALA-5905: add script for all-build-options job
..


IMPALA-5905: add script for all-build-options job

This checks in a modified version of the job script for
https://jenkins.impala.io/view/Experimental/job/all-build-options
which adds UBSAN and TSAN.

The script is also modified to not reference any jenkins environment
variables, e.g. WORKSPACE, since the Jenkins job script intermingled
references to those with the script logic.

Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Reviewed-on: http://gerrit.cloudera.org:8080/8043
Reviewed-by: Tim Armstrong 
Tested-by: Tim Armstrong 
---
A bin/jenkins/build-all-flag-combinations.sh
1 file changed, 59 insertions(+), 0 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved; Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5905: add script for all-build-options job
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:

Line 1: #!/usr/bin/env bash
> I named the Jenkins job "all-build-options". I'm OK with a name change of a
Done


Line 21: # succeeds. Intended for use as a precommit test to make sure nothing 
got broken.
> Required the ninja build system and ccache to be installed, which are not s
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job