Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )
Change subject: IMPALA-5908: Allow SET to unset modified query options. ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h@347 PS10, Line 347: TQueryOptions *server_default_query_options; > nit: placement of * Done http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc@134 PS7, Line 134: NULL > nullptr (we still have NULL all over the codebase but we're gradually switc Done http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@a61 PS10, Line 61: > Ahh we've seen this mistake a few times before. Ack http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@83 PS10, Line 83: > nit: placement of * Done http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift@86 PS10, Line 86: a > a what? done. "a user" http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py File tests/custom_cluster/test_set_and_unset.py: http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py@52 PS9, Line 52: # Use a "request overlay" to change the option for a specific : # request within a session. We run a r > What do you want to do about this? I'm okay with submitting this patch as- I think these semantics are plausible. It's sensible to have a command to return the current session options, which is what this does. I think we can leave it alone. http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py File tests/custom_cluster/test_set_and_unset.py: http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py@25 PS10, Line 25: class TestSetAndUnset(CustomClusterTestSuite, HS2TestSuite): > Thanks for adding this, clearly there was a gap in test coverage here. Ack -- To view, visit http://gerrit.cloudera.org:8080/8070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9 Gerrit-Change-Number: 8070 Gerrit-PatchSet: 10 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 29 Sep 2017 20:03:53 +0000 Gerrit-HasComments: Yes
