Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22718 )
Change subject: IMPALA-13923: Support more compression levels for ZSTD and ZLIB ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/runtime/tmp-file-mgr.h@368 PS3, Line 368: /// and ignored otherwise. -1 means not set/invalid. Please update the comment to reflect support for negative levels. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options-test.cc@626 PS3, Line 626: // Test with compression levels for all codecs Is this really "all codecs"? It doesn't cover snappy, lz4, etc. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options-test.cc@652 PS3, Line 652: // Test minimum negative comression level typo: comression -> compression http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options-test.cc@657 PS3, Line 657: // Unnecessary to test range of such low values, begin from - This sentence seems incomplete. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/service/query-options.cc@310 PS3, Line 310: std::optional<int> compression_level = std::nullopt; nit: this is the default value, explicit initialization to nullopt isn't needed. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/parse-util.h File be/src/util/parse-util.h: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/parse-util.h@74 PS3, Line 74: std::optional<int>& level); As a general practice, the Impala code base prefers pointers for values that may be modified by the function. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/parse-util.cc File be/src/util/parse-util.cc: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/parse-util.cc@173 PS3, Line 173: return Status(Substitute("Invalid compression level '$0' for codec $1.", I have a slight preference for providing the error message at each site and customizing it to provide better guidance. Like 1. Compression level is not an integer. 2. Compression level must be in the range $ZSTD_minCLevel to $ZSTD_maxCLevel 3. Compression level must be in the range $Z_DEFAULT_COMPRESSION to $Z_BEST_COMPRESSION. -- To view, visit http://gerrit.cloudera.org:8080/22718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b98c735246f08e04598a4e752c8cca04e31a88a Gerrit-Change-Number: 22718 Gerrit-PatchSet: 3 Gerrit-Owner: Surya Hebbar <sheb...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Surya Hebbar <sheb...@cloudera.com> Gerrit-Comment-Date: Tue, 15 Apr 2025 20:04:48 +0000 Gerrit-HasComments: Yes
