Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19096 )
Change subject: IMPALA-11537: Query option validation for numeric types ...................................................................... Patch Set 11: Code-Review+1 (3 comments) Thanks for the changes, added a few nitpicks otherwise looks good to me. http://gerrit.cloudera.org:8080/#/c/19096/11/be/src/util/parse-util.h File be/src/util/parse-util.h: http://gerrit.cloudera.org:8080/#/c/19096/11/be/src/util/parse-util.h@33 PS11, Line 33: int64_t value = 0; : const std::string* original_value = nullptr; : : MemSpec() = default; : MemSpec(int64_t value) : value(value) {} : MemSpec(int64_t value, const std::string* original_value) You never actually call the constructors explicitly. You either value initialze, e.g.: return {{mem_spec, &value}, is_parse_ok}; RETURN_IF_ERROR(QueryOptionValidator<MemSpec>::InclusiveRange( option, mem_spec_val, {0}, {SPILLABLE_BUFFER_LIMIT})); or you call the one parameter constructor implicitly, e.g: RETURN_IF_ERROR(QueryOptionParser::ParseAndCheckInclusiveRange<MemSpec>(option, value, 1, ROW_SIZE_LIMIT, &mem_spec_val)); We could clean this a bit up with using exclusively value initialization, as we do not have any invariant: struct MemSpec { int64_t value {}; // see my comment below // const std::string* original_value {}; // no construction definition needed, compiler does it ... } and replacing all the implicit construction calls in function parameters with value initialization, i.e. {0} instead of 0. This need one more change to work, in be/src/service/query-option-parser.h::82 T{0} (which does make more sense, and happened implicitly) http://gerrit.cloudera.org:8080/#/c/19096/11/be/src/util/parse-util.h@48 PS11, Line 48: os << strings::Substitute("$0 ($1)", *mem_spec.original_value, mem_spec.value); Is it always guaranteed that *original_value outlives the MemSpec object? Currently yes, as I see we propagate only MemSpec::value, but this can cause subtle bugs. I would add rather not log the parsed string (the user can see it in his history) and just log the returned integer (or Inf, see my other comment). http://gerrit.cloudera.org:8080/#/c/19096/11/testdata/workloads/functional-query/queries/QueryTest/set.test File testdata/workloads/functional-query/queries/QueryTest/set.test: http://gerrit.cloudera.org:8080/#/c/19096/11/testdata/workloads/functional-query/queries/QueryTest/set.test@268 PS11, Line 268: ---- CATCH : Invalid value for query option MAX_ROW_SIZE: -1 (0) is not in range [1, 1099511627776] nit: This was already a bit confusing, -1 MemSpec meaning infinity but represented as 0, but my proposed change made it worse. Maybe we could handle the value==0 case in the operator<< and print Inf -- 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: 11 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Thu, 10 Nov 2022 18:04:20 +0000 Gerrit-HasComments: Yes
