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 <vtt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vincent Tran <vtt...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-HasComments: Yes
