impala-reviews mailing list archives

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

Mime
View raw message