impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.
Date Thu, 14 Sep 2017 20:21:54 GMT
Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8070/1//COMMIT_MSG
Commit Message:

Line 9: The query 'SET <option>=""' will now unset option, reverting it to
> A quick question, did you consider other alternatives like "UNSET <option>"
I did, and discussed it somewhat in the JIRA.

Our shell has 'unset' functionality today, but the server-side state doesn't. I think on balance
having one command is going to have less complexity.

Interestingly, the confusion with compression_codec was what spurred the issue that caused
this. In IMPALA-5589, it looked like resetting the setting to its default was NONE, which
wasn't the right default value.

(This is all a cautionary tale about treating "unset" differently in Thrift...)


http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
> Let's default to using 'static' -- it's concise. It's also been un-deprecia
https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables
agrees that 'static' and anonymous namespaces are both cool. I've left this alone.


Line 133:     if (value == "") {
> Nit: Maybe make this an "else if" to avoid the extra indentation. This woul
Done


Line 137:         case TImpalaQueryOptions::ABORT_ON_ERROR:
> I did a pass over the query options to check that the new behaviour made se
I agree. I'm adding a note to common/thrift/ImpalaInternalService.thrift to try to explain
a caveat.

I looked over the documentation and I didn't see any information about UNSET outside of impala-shell.

I filed https://issues.apache.org/jira/browse/IMPALA-5937 to add documentation for a few query
options that are missing it.


Line 196:           if (value.empty()) break;
> This is dead now I think.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message