Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20496 )
Change subject: IMPALA-12373: Small String Optimization for StringValue ...................................................................... Patch Set 16: Code-Review+1 (1 comment) The implementation looks good to me, but I am still concerned about test coverage issues. http://gerrit.cloudera.org:8080/#/c/20496/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20496/4//COMMIT_MSG@86 PS4, Line 86: | > I extended my TODO-list for now. I am still concerned about reducing coverage of copying strings. For example in https://gerrit.cloudera.org/#/c/20108/ the test data with nested types would have to be extended to have coverage both for small and large strings in the new code. I think that the ideal solution would be to have a way to disable short string optimization and run the tests in both ways (with the exception of the few tests where the results are changed, e.g. it changes mem estimates). For example normal tests could be run with optimization turned on, while docker tests would turn it off. My guess is that adding a flag wouldn't add noticeable overhead, and this would also let us disable this later if it turns out that there are some bugs introduced. Is it ok for you to proceed like this? So: - add a flag and run the benchmarks a. if it doesn't add much overhead, disable the flag in docker builds b. if it adds overhead, think about solution b :) -- To view, visit http://gerrit.cloudera.org:8080/20496 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I741c3a5f12ab620b6b64b57d4c89b5f8e056efd3 Gerrit-Change-Number: 20496 Gerrit-PatchSet: 16 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 20 Nov 2023 10:09:03 +0000 Gerrit-HasComments: Yes
