impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
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:

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...)
File be/src/service/

Line 113: static int GetQueryOptionForKey(const string& key) {
> Let's default to using 'static' -- it's concise. It's also been un-deprecia
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

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 to add documentation for a few query
options that are missing it.

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message