Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19096 )
Change subject: IMPALA-11537: Query option validation numeric types ...................................................................... Patch Set 4: (17 comments) http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h File be/src/service/query-option-parser.h: http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@49 PS4, Line 49: error_handling Should be 'validation_status' or something similar. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@55 PS4, Line 55: static Status ParseAndApply(TImpalaQueryOptions::type option, const std::string& value, Nit: add empty line before function. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@63 PS4, Line 63: return {TErrorCode::QUERY_OPTION_PARSE_FAILED, I think an explicit constructor call like Status(TErrorCode::QUERY_OPTION_PARSE_FAILED, _TImpalaQueryOptions_VALUES_TO_NAMES.at(option), value) would be more readable. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@113 PS4, Line 113: bool is_parse_ok = !(mem_spec.value < 0); Nit: optional, mem_spec.value >= 0 http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@157 PS4, Line 157: s Nit: equal http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@157 PS4, Line 157: Nit: 'actual value is $1' OR you could use a colon (:) as on L175. The format of the message here and in the lower bound check should be consistent. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@166 PS4, Line 166: Nit: See L157. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@175 PS4, Line 175: s Nit: equal http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@192 PS4, Line 192: return Status(strings::Substitute("Value can't be $0", other)); Also include the actual value in the error message. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-option-parser.h@199 PS4, Line 199: Nit: colon (:) http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc@199 PS4, Line 199: GetQueryOptionForKey Why call GetQueryOptionForKey() twice? I think we should first put it into an int variable (e.g. option_int), check it, then convert that to TImpalaQueryOptions::type. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc@205 PS4, Line 205: auto Nit: I think writing the actual type makes it easier to read the code. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc@219 PS4, Line 219: RETURN_IF_ERROR(QueryOptionParser<int64_t>::ParseAndApply( : option, value, [query_options](int setter_value) { : query_options->__set_max_errors(setter_value); : })); > warning, matter of taste :) Yes, a matter of taste :) I don't really like the idea of reusable variables before the switch block. Even if we take Csaba's approach, I would still prefer the variables to be local in the case sections despite that adding more lines. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc@400 PS4, Line 400: return Status::OK(); No need for an 'else' block because we return in the 'if'. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/service/query-options.cc@943 PS4, Line 943: [](auto input) { Could use InclusiveRange() instead of lower and upper bound. http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/util/parse-util.cc File be/src/util/parse-util.cc: http://gerrit.cloudera.org:8080/#/c/19096/4/be/src/util/parse-util.cc@41 PS4, Line 41: ParseMemSpec(mem_spec_str, &is_percent, relative_reference) If we ever change this to a constructor call (instead of aggregate initialization), the order of evaluating the arguments would be unspecified (at least in some C++ versions) which could lead to a bug. For aggregate initialization the order is defined so this should be ok, but it would be safer if we put the ParseMemSpec() call to a variable and used that in initializing the return value. http://gerrit.cloudera.org:8080/#/c/19096/4/testdata/workloads/functional-query/queries/QueryTest/set.test File testdata/workloads/functional-query/queries/QueryTest/set.test: http://gerrit.cloudera.org:8080/#/c/19096/4/testdata/workloads/functional-query/queries/QueryTest/set.test@269 PS4, Line 269: inclusive > To me [ ] already suggests an inclusive range, so an exclusive range would Yes, though I'm not sure if this convention is universal. I'd say it's better to be explicit here but I accept if you don't agree. -- To view, visit http://gerrit.cloudera.org:8080/19096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7757b52393c094d2c661918d73cbfad7214f855 Gerrit-Change-Number: 19096 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Tue, 25 Oct 2022 11:49:20 +0000 Gerrit-HasComments: Yes
