impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5052: Read and write signed integer logical types in Parquet
Date Fri, 05 Jan 2018 19:09:19 GMT
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 <aphadke@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Jan 2018 19:09:19 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message