You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vincent Tran (Code Review)" <ge...@cloudera.org> on 2017/06/17 19:14:45 UTC
[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 <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/851/
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
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 <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7219/1//COMMIT_MSG
Commit Message:
PS1, Line 7: DISABLE_UNSAFE_SPILLS
> typo
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 <vt...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
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 <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 5:
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
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 <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran has uploaded a new patch set (#2).
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_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/2
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLS=1
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 2:
(10 comments)
Thanks for the patch. I have a lot of minor comments for cleanup but nothing major.
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
&& queryCtx.isSetTables_missing_stats()
&& !queryCtx.tables_missing_stats.isEmpty()
In case tables_missing is set to an empty list.
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 query is compute stats and the other is select *?
PS2, Line 435: request_with_disable_spill_off
Variables in the fe are capitalised like: requestWithDisableSpillOff
Line 437: try {
You can just remove the try()/catch(). If an exception is thrown and not caught it will fail the test. You could just add a comment to explain what bug it's testing for.
Line 438: request_with_disable_spill_off = frontend_.createExecRequest(queryCtx, explainBuilder);
Long line - can you wrap to 90 characters?
PS2, Line 442: Preconditions.checkNotNull
Use JUnit's assertNotNull() in tests
Line 445: try {
Same here - the try/catch isn't really necessary - we can make the test a bit shorter.
Line 446: request_with_disable_spill_on = frontend_.createExecRequest(queryCtx, explainBuilder);
Long line - can you wrap to 90 characters?
Line 448: Assert.fail("Failed to create exec request with DISABLE_UNSAFE_SPILLS=true:" + e.getMessage());
Long line - can you wrap to 90 characters?
Line 450: Preconditions.checkNotNull(request_with_disable_spill_on);
Use JUnit's assertNotNull() in tests
--
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 <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_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
Reviewed-on: http://gerrit.cloudera.org:8080/7219
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
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(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iccae7fdeaf0ade0b8728a7d249981c8270e8c327
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
Patch Set 5: Code-Review+2
Looks good, thanks Vincent!
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLS=1
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran has uploaded a new patch set (#4).
Change subject: IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_SPILLS=1
......................................................................
IMPALA-5524: Fixes NPE during planning with DISABLE_UNSAFE_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/4
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>