Lars Volker has posted comments on this change. Change subject: IMPALA-3376: Extra definition level when writing Parquet files ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/3556/2/be/src/util/parquet-reader.cc File be/src/util/parquet-reader.cc: Line 149: uint8_t decompressed_buffer[header.uncompressed_page_size]; This looks odd to me, I think C++ requires constant array sizes for arrays on the stack. The buffer allocated in L214 also looks like it's leaking. Probably both don't really matter in practice as the program terminates quickly and memory gets free'd. However you might want to fix it. Two nicer ways to do so would be using std::unique_ptr(new[] ...) or use a std::vector<uint8_t> as a buffer. I don't know if either one is preferable over the other. http://gerrit.cloudera.org:8080/#/c/3556/1/tests/query_test/test_writers.py File tests/query_test/test_writers.py: PS1, Line 24: > We're inconsistent about this - a quick grep through the python tests looks I generally try to follow PEP8, specifically this section on https://www.python.org/dev/peps/pep-0008/#documentation-strings http://gerrit.cloudera.org:8080/#/c/3556/2/tests/query_test/test_writers.py File tests/query_test/test_writers.py: Line 48: check_call(['hdfs', 'dfs', '-copyToLocal', '/hdfs_parquet_table_writer', tmp_dir]) If this fails you'll leak the temporary directory. Move this line into the try/finally? Line 50: for f in os.listdir(os.path.join(tmp_dir, 'hdfs_parquet_table_writer/')): This will not recurse into subdirectories. If hdfs_parquet_table_writer doesn't have any subdirectories, you could add that as a comment. -- To view, visit http://gerrit.cloudera.org:8080/3556 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2cafd7ef6b607ce6f815072b8af7395a892704d9 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
