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 <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
