impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vincent Tran (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNFASE SPILLSö0A
Date Fri, 30 Jun 2017 12:53:44 GMT
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 <vttran@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vttran@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message