Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12682 )
Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@343 PS2, Line 343: uery_options->__set_replica_preference(T > Yeah this is an odd one because we don't remove the unused options, .i.e. C I see. There are some other query options where not all values are actually supported, e.g. I am sure that parquet_compression_codec will only work with a subset of codecs. default_file_format is also questionable, as Impala can create all kinds of tables, but cannot insert into some of them, so I am not sure if it makes sense to allow those as defaults. I am ok with the current state of the patch, but it may make sense to filter unsupported enums, e.g. by adding an optional argument to GetThriftEnum with the list of valid values. http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@365 PS2, Line 365: Specifying the template does not seem necessary here, as it can be deduced from argument 'enum_type'. -- To view, visit http://gerrit.cloudera.org:8080/12682 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641 Gerrit-Change-Number: 12682 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 08 Mar 2019 17:17:02 +0000 Gerrit-HasComments: Yes
