Tim Armstrong has posted comments on this change. Change subject: IMPALA-5627: fix dropped statuses in HDFS writers ......................................................................
Patch Set 4: (4 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? I intended to add it to the base class but missed doing that. Thanks for catching. Added that and then added "override" declarations here so it's clearer that they're overridden functions. 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_? Done 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 It always is used. Updated. I'd prefer not to document where the variable is set in this case since I think it will get out of sync with the code too easily, so you need to grep the code anyway to verify that the comment is correct. Instead expanded a little on why it is set. 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()? Done -- 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
