Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8548 )
Change subject: IMPALA-5052: Read and write signed integer logical types in Parquet ...................................................................... Patch Set 2: Code-Review+1 (4 comments) Thanks, this is looking good. Couple of minor comments then I'll let tianyi +2 http://gerrit.cloudera.org:8080/#/c/8548/2/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/8548/2/tests/query_test/test_insert_parquet.py@310 PS2, Line 310: if line_split[0] == "id": Can you make this an elif chain and assert that the column name is one of the expected column name. Just to make it less likely that an error in the tests results in it silently failing. I.e. something like if line_split[0] == 'id': ... elif line_split[0] == 'int': ... else: assert line_split[0] == 'bigint_col' ... http://gerrit.cloudera.org:8080/#/c/8548/2/tests/query_test/test_insert_parquet.py@339 PS2, Line 339: if line_split[0] == "tinyint_col": Same here http://gerrit.cloudera.org:8080/#/c/8548/2/tests/query_test/test_insert_parquet.py@349 PS2, Line 349: %src_tbl) nit: missing space after % http://gerrit.cloudera.org:8080/#/c/8548/2/tests/query_test/test_insert_parquet.py@351 PS2, Line 351: % same here -- To view, visit http://gerrit.cloudera.org:8080/8548 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I47a8371858c9597c6a440808cf6f933532468927 Gerrit-Change-Number: 8548 Gerrit-PatchSet: 2 Gerrit-Owner: anujphadke <apha...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Fri, 05 Jan 2018 19:09:19 +0000 Gerrit-HasComments: Yes
