Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12635 )
Change subject: IMPALA-8254: Fix error when running compute stats with compression_codec set ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12635/3/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/12635/3/be/src/service/query-options.cc@235 PS3, Line 235: if (iequals(value, "none") || iequals(value, "0")) { optional: these big else if chains look error prone - it would be probably safer to create a macro or template function that takes 'value' and the VALUES_TO_NAMES map of a thrift enum (_THdfsCompression_VALUES_TO_NAMES in this case), iterates the map, and returns the enum where the name or the number matches. This would have the side effect that if the .thrift file is updated and a new value is added to the enum, then this new value would be recognized as query option automatically. The list of enum values accepted by Impala could be also added if this is a problem. http://gerrit.cloudera.org:8080/#/c/12635/3/tests/metadata/test_compute_stats.py File tests/metadata/test_compute_stats.py: http://gerrit.cloudera.org:8080/#/c/12635/3/tests/metadata/test_compute_stats.py@79 PS3, Line 79: @pytest.mark.execute_serially Does this have to be executed serially? It should not collide with other tests because of the unique_database. -- To view, visit http://gerrit.cloudera.org:8080/12635 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2cb546fbd3d2a02e0ed30d85a33a04852bed9dd2 Gerrit-Change-Number: 12635 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 28 Feb 2019 13:42:33 +0000 Gerrit-HasComments: Yes
