Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9381 )
Change subject: IMPALA-6542: Fix inconsistent write path of Parquet min/max statistics ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/9381/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9381/5//COMMIT_MSG@12 PS5, Line 12: ignores > If two NaN's are inserted, it wouldn't ignore them but write them into the Yeah, the next sentence describes that case. Let me rephrase it. I'm not sure if this is the best option, but it was intentional. I had a discussion with the parquet-cpp community about the behavior of the quick fix. We agreed to follow what fmax/fmin does: they only return NaN when both arguments are NaNs. IMPALA-6539 is about the long-term fix, but it can't be implemented until PARQUET-1222 is resolved. http://gerrit.cloudera.org:8080/#/c/9381/5/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test: http://gerrit.cloudera.org:8080/#/c/9381/5/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@580 PS5, Line 580: ==== > Can you add a test that inserts multiple NaNs, with and without following t Added some tests for these cases. -- To view, visit http://gerrit.cloudera.org:8080/9381 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3957806948f7c661af4be5495f2ec92d1e9fc9d6 Gerrit-Change-Number: 9381 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Mar 2018 17:25:56 +0000 Gerrit-HasComments: Yes
