Pooja Nilangekar has posted comments on this change. Change subject: IMPALA-5061: Populate null_count in parquet::statistics ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/7058/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 961: Status err(Substitute("Corrupt Parquet file '$0': column '$1' " > If possible, please don't rebase changes while they are under review. Other Sorry about that! I won't do it henceforth. http://gerrit.cloudera.org:8080/#/c/7058/5/be/src/exec/hdfs-parquet-table-writer.cc File be/src/exec/hdfs-parquet-table-writer.cc: PS5, Line 197: page_stats_base_ > can this ever be nullptr? I am not sure, I noticed Line #688 does a DCHECK. Should I change it to a DCHECK or entirely remove this line? http://gerrit.cloudera.org:8080/#/c/7058/5/be/src/exec/parquet-column-stats.cc File be/src/exec/parquet-column-stats.cc: Line 58: DCHECK(false) << "Unsupported statistics field requested"; > undo Done http://gerrit.cloudera.org:8080/#/c/7058/5/be/src/exec/parquet-column-stats.h File be/src/exec/parquet-column-stats.h: Line 60: /// Enum to select whether to read minimum or maximum statistics. Values do not > nit: Please undo this change, too. Done Line 72: static bool ReadFromThrift(const parquet::ColumnChunk& col_chunk, > Undo Done Line 100: void IncrementNullCount(int64_t count) { null_count_ += count; } > Update sounds like it will set it to a new value. Maybe "AddToNullCount" or Done. Changed it to IncrementNullCount(int64_t count) Line 162: void Update(const T& min_value, const T& max_value); > You can remove the implementation of Update(const T& v) and instead put a s Done Line 163: > Please add a comment to this method, too. Done http://gerrit.cloudera.org:8080/#/c/7058/5/be/src/exec/parquet-column-stats.inline.h File be/src/exec/parquet-column-stats.inline.h: Line 61: std::string min_str; > nit: This could go on a single line now Done -- To view, visit http://gerrit.cloudera.org:8080/7058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c49a63af84c2234f0633be63206cb52eb7e8ebb Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-HasComments: Yes
