Lars Volker has posted comments on this change. Change subject: IMPALA-5627: fix dropped statuses in HDFS writers ......................................................................
Patch Set 4: (4 comments) Thank you for fixing this! Please see my inline comments. http://gerrit.cloudera.org:8080/#/c/7372/4/be/src/exec/hdfs-avro-table-writer.h File be/src/exec/hdfs-avro-table-writer.h: Line 71: virtual Status Init(); Can we add WARN_UNUSED_RESULT to virtual functions, too? http://gerrit.cloudera.org:8080/#/c/7372/4/be/src/exec/hdfs-parquet-table-writer.cc File be/src/exec/hdfs-parquet-table-writer.cc: Line 194: // aggregates. Returns true if the value was appended successfully to the current page. Should this also mention that the method updates next_page_encoding_? Line 260: Encoding::type next_page_encoding_; // Preferred encoding for the next page. Is this ever not used as the encoding of the next page? Otherwise it may be clearer to just say "Encoding for the next page". Can you also add a sentence where this variable is set? Nit: Can you also put the comments above the variables? I personally find it easier to read and more consistent with the rest of the code, but I also don't feel strongly about it. http://gerrit.cloudera.org:8080/#/c/7372/4/be/src/exec/parquet-column-stats.cc File be/src/exec/parquet-column-stats.cc: Line 123: RETURN_IF_ERROR(buffer->Append(value->ptr, value->len)); can you add WARN_IF_UNUSED to StringBuffer::Append()? -- To view, visit http://gerrit.cloudera.org:8080/7372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I753d352c640faf5eaef650cd743e53de53761431 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
