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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
