Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19096 )
Change subject: IMPALA-11537: Query option validation for numeric types ...................................................................... Patch Set 6: (23 comments) http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h File be/src/service/query-option-parser.h: http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@112 PS6, Line 112: if (!BitUtil::IsPowerOf2(value)) { > Is there a reason why we don't write impala::BitUtil::IsPowerOf2() instead Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@142 PS6, Line 142: & > The convention in Impala is that we take output parameters by pointer, not Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@152 PS6, Line 152: & > See L142. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@154 PS6, Line 154: if (!status.ok()) { > We could use the RETURN_IF_ERROR macro instead on L153. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@161 PS6, Line 161: & > See L142. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@170 PS6, Line 170: & > See L142. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@173 PS6, Line 173: return status; > See L154. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@179 PS6, Line 179: & > See L142. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@181 PS6, Line 181: if (!status.ok()) { > See L154. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-option-parser.h@214 PS6, Line 214: template <typename Memspec, > Can't we use a simple template specialisation for MemSpec? In this case, to the best of my knowledge, we can't. If the MemSpec case would be defined without second level of templating, the calls to ParseInternal would all come to this case, regardless of the class template type. If the calling would have the second parameter as well (like how it's done now: ParseInternal<T>), the MemSpec case would not match to that call, because there would be no second-level template argument. http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@407 PS6, Line 407: numeric_limits<int32_t>::max() > This will always be true because int32_val is an int32_t. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@534 PS6, Line 534: option, value, int32_t_val)); > Nit: indentation should be +4. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@566 PS6, Line 566: option, value, int32_t_val)); > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@588 PS6, Line 588: option, value, int32_t_val)); > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@606 PS6, Line 606: > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@606 PS6, Line 606: numeric_limits<int32_t>::max() > Always true. See L407. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@613 PS6, Line 613: numeric_limits<int32_t>::max() > Always true. See L407. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@613 PS6, Line 613: > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@755 PS6, Line 755: numeric_limits<int32_t>::max() > Always true. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/service/query-options.cc@996 PS6, Line 996: std::numeric_limits<int32_t>::max() > Always true. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/util/parse-util.h File be/src/util/parse-util.h: http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/util/parse-util.h@34 PS6, Line 34: int64_t value{}; : bool is_percent{}; > Giving 0 and false explicitly would be more informative. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/util/parse-util.h@37 PS6, Line 37: : > Nit: spaces around :. Done http://gerrit.cloudera.org:8080/#/c/19096/6/be/src/util/parse-util.h@38 PS6, Line 38: :value(value),is_percent(is_percent){} > Nit: spaces around colon and comma, soace before {}. Done -- 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: 6 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: Fri, 28 Oct 2022 11:23:33 +0000 Gerrit-HasComments: Yes
