[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-07-11 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..


Patch Set 3:

Did you have a chance to have another look at this? Would be good to get the 
change in and fix the bug.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-07-06 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..


Patch Set 3:

(2 comments)

Looks good to me. If you fix the typo in the commit message and rebase then I 
can start the merge.

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

PS3, Line 7: DISABLE_UNFASE_SPILLS
UNSAFE


http://gerrit.cloudera.org:8080/#/c/7219/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:

Line 437:   @Test
> Done
I believe JUnit test methods can be declared to throw exceptions. JUnit 
definitely catches anything thrown by tests and fails that individual test with 
an error.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-06-30 Thread Vincent Tran (Code Review) 
Vincent Tran has posted comments on this change.

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7219/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

Line 1027:   && queryCtx.isSetTables_missing_stats()
> I think this should be
Done


http://gerrit.cloudera.org:8080/#/c/7219/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:

Line 432: queryCtx.client_request.setStmt("compute stats 
functional.alltypes");
> It might help to explain the choice of queries. Is it significant that one 
Done


PS2, Line 435: request_with_disable_spill_off
> Variables in the fe are capitalised like: requestWithDisableSpillOff
Done


Line 437: try {
> You can just remove the try()/catch(). If an exception is thrown and not ca
Done
frontend_.createExecRequest() throws, so we either have to catch it or the test 
method have to throw as well. Is there any implication within the unit test 
framework? (i.e. is it possible that it may halt all of the tests?)


Line 438:   request_with_disable_spill_off = 
frontend_.createExecRequest(queryCtx, explainBuilder);
> Long line - can you wrap to 90 characters?
Done


PS2, Line 442: Preconditions.checkNotNull
> Use JUnit's assertNotNull() in tests
Done


Line 445: try {
> Same here - the try/catch isn't really necessary - we can make the test a b
Done


Line 446:   request_with_disable_spill_on = 
frontend_.createExecRequest(queryCtx, explainBuilder);
> Long line - can you wrap to 90 characters?
Done


Line 448:   Assert.fail("Failed to create exec request with 
DISABLE_UNSAFE_SPILLS=true:" + e.getMessage());
> Long line - can you wrap to 90 characters?
Done


Line 450: Preconditions.checkNotNull(request_with_disable_spill_on);
> Use JUnit's assertNotNull() in tests
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-06-30 Thread Vincent Tran (Code Review) 
Vincent Tran has uploaded a new patch set (#3).

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..

IMPALA-5524: Fixes NPE during planning with DISABLE_UNFASE_SPILLS=1

This change will avoid a NPE during query planning under the following
conditions:
- DISABLE_UNSAFE_SPILLS is TRUE or 1
- All tables involved in the query have stats

Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
2 files changed, 20 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-06-18 Thread anujphadke (Code Review) 
anujphadke has posted comments on this change.

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..


Patch Set 1:

(1 comment)

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

PS1, Line 7: DISABLE_UNFASE_SPILLS
typo


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1

2017-06-17 Thread Vincent Tran (Code Review) 
Vincent Tran has uploaded a new change for review.

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

Change subject: IMPALA-5524: Fixes NPE during planning with 
DISABLE_UNFASE_SPILLS=1
..

IMPALA-5524: Fixes NPE during planning with DISABLE_UNFASE_SPILLS=1

This change will avoid a NPE during query planning under the following
conditions:
- DISABLE_UNSAFE_SPILLS is TRUE or 1
- All tables involved in the query have stats

Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
2 files changed, 26 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran