impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5627: fix dropped statuses in HDFS writers
Date Thu, 13 Jul 2017 01:27:38 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message