Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11000 )
Change subject: IMPALA-5542: Impala cannot scan Parquet decimal stored as int64_t/int32_t ...................................................................... Patch Set 1: (2 comments) My main question arose out of me trying to understand the implications of the changes to ByteSize() - I feel like there's something weird there. http://gerrit.cloudera.org:8080/#/c/11000/1/be/src/exec/parquet-common.h File be/src/exec/parquet-common.h: http://gerrit.cloudera.org:8080/#/c/11000/1/be/src/exec/parquet-common.h@62 PS1, Line 62: static int ByteSize(const InternalType& v) { return sizeof(InternalType); } I'm still confused about what this function does. Can we rename to something like "InternalByteSize()". What does it returning -1 mean also? It's weird that it also sometimes seems to be used to return the encoded byte size - that seems wrong. E.g. I'm looking at this logic in HdfsParquetTableWriter and wondering if it's wrong: *bytes_needed = plain_encoded_value_size_ < 0 ? ParquetPlainEncoder::ByteSize<T>(*v) : plain_encoded_value_size_; http://gerrit.cloudera.org:8080/#/c/11000/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/11000/1/tests/query_test/test_scanners.py@293 PS1, Line 293: def _create_table_from_file(self, table_name, unique_database): It would be nice to use this for other tests (no requirement, just an observation :)). -- To view, visit http://gerrit.cloudera.org:8080/11000 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib8c41bfc7c1664bdba5099d3893dc8dbe4304794 Gerrit-Change-Number: 11000 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 20 Jul 2018 16:07:57 +0000 Gerrit-HasComments: Yes
