impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5524: Fixes NPE during planning with DISABLE UNSAFE SPILLSö0A
Date Tue, 20 Jun 2017 05:44:31 GMT
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 <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