Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12004 )
Change subject: IMPALA-7889: Write new logical types in Parquet ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/12004/7/be/src/exec/parquet/parquet-metadata-utils.cc File be/src/exec/parquet/parquet-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/12004/7/be/src/exec/parquet/parquet-metadata-utils.cc@158 PS7, Line 158: case parquet::Encoding::RLE: > There's already a "IsSupportedType()" in the anonymous namespace above, I t Done http://gerrit.cloudera.org:8080/#/c/12004/7/be/src/exec/parquet/parquet-metadata-utils.cc@290 PS7, Line 290: } > I'd consider moving all anonymous helpers up into one anonymous namespace Done http://gerrit.cloudera.org:8080/#/c/12004/5/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/12004/5/tests/query_test/test_insert_parquet.py@388 PS5, Line 388: schema = self._find_schema(schemas, column_name) > You could rename _find_schema to _get_schema() if you feel that that would Done http://gerrit.cloudera.org:8080/#/c/12004/7/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/12004/7/tests/query_test/test_insert_parquet.py@37 PS7, Line 37: from tests.util.get_parquet_metadata import get_parquet_metadata, decode_stats_value, \ > nit: Wrap these in parentheses, and while you're here the ones above, too. Done http://gerrit.cloudera.org:8080/#/c/12004/7/tests/query_test/test_insert_parquet.py@388 PS7, Line 388: schema = self._find_schema(schemas, column_name) > I think you can shorten this to something like: Thank, it is nicer this way. http://gerrit.cloudera.org:8080/#/c/12004/7/tests/query_test/test_insert_parquet.py@410 PS7, Line 410: > nit: I think we indent 4 spaces here You are right, I also changed indentation at some other places. http://gerrit.cloudera.org:8080/#/c/12004/7/tests/query_test/test_insert_parquet.py@456 PS7, Line 456: > Is there actually a Jira we can add here? If not that's ok, too IMPALA-5049 is mentioned below - is that not enough? -- To view, visit http://gerrit.cloudera.org:8080/12004 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6f377950845683ab9c6dea79f4c54db0359d0b91 Gerrit-Change-Number: 12004 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Thu, 13 Dec 2018 17:53:19 +0000 Gerrit-HasComments: Yes
