Alex Behm has posted comments on this change. Change subject: IMPALA-5250: Unify decompressor output_length semantics ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/8030/1//COMMIT_MSG Commit Message: Line 26: instead of the "fast" version, for the latter is insecure with corrupted > Is there an individual benchmark? Not sure how useful compression-test.cc is. I was thinking more along the lines of end-to-end tests that have heavy data exchanges. http://gerrit.cloudera.org:8080/#/c/8030/1/be/src/util/decompress-test.cc File be/src/util/decompress-test.cc: Line 196: } > I'm not sure if my interpretation of JIRA is correct. In my understanding: Your interpretation is correct. I'm just referring to a specific broken file I've seen where I believe the output_len should be 0. Can you please make sure that your change fixes the bug for that specific broken file and check whether these tests cover that scenario? http://gerrit.cloudera.org:8080/#/c/8030/1/be/src/util/decompress.cc File be/src/util/decompress.cc: Line 426: // If size_only is false, output buffer size must be at least output_len. *output_len is > Done. It is set to 0 in this case, while other decompressor may leave it un Agree this in/out pattern is not great and we should consider your proposal (but not in this patch). I'm ok with setting to 0 everywhere, but it might be easier to just leave output_len untouched. I saw in the code that in some places it is set to 0, but not in others. My main ask is for consistency and documenting the behavior in a comment. -- To view, visit http://gerrit.cloudera.org:8080/8030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-HasComments: Yes
