[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
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
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
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
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
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
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