Tim Armstrong has posted comments on this change. Change subject: IMPALA-5908: Allow SET to unset modified query options. ......................................................................
Patch Set 1: (4 comments) Makes sense to me. Had some minor comments but I'd also like someone else to confirm that the new behaviour makes sense. 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) { Nice catch. I don't feel strongly either about this (static makes more sense to me) but putting functions in an anonymous namespace is the C++-ish way to prevent the symbols from being exported. The codebase hasn't really converged to standardise on one or the other. Line 133: if (value == "") { Nit: Maybe make this an "else if" to avoid the extra indentation. This would also make the diff look less scary :). Line 137: case TImpalaQueryOptions::ABORT_ON_ERROR: I did a pass over the query options to check that the new behaviour made sense. I think in almost every case either the behaviour is unchanged or the option did not accept "" as input. We were a bit lucky there in the the default value of many options is 0 and atoi() returns 0 for invalid input like an empty string. The behaviour changes I found were: * compression_codec, which used to just ignore the "set" command if it was an empty string, which seems wrong * exec_single_node_rows_threshold, which defaults to 100 but interprets "" as 0. * parquet_dictionary_filtering and parquet_read_statistics, which default to true but interpret "" as false I don't think any of the above behaviour is documented or makes much sense so it seems fine to change it. Would be good to have someone confirm this reasoning though. Line 196: if (value.empty()) break; This is dead now I think. -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
