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] Preview: IMPALA-4363: Add timestamp validation
Date Sun, 06 Nov 2016 06:58:43 GMT
Lars Volker has posted comments on this change.

Change subject: Preview: IMPALA-4363: Add timestamp validation
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4968/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:

Line 44: 
?


Line 491:   inline bool NeedsValidation() const {
Can you add a brief comment?


Line 502:   /// Verifies that data is acceptable
Can you elaborate what "acceptable" means?


http://gerrit.cloudera.org:8080/#/c/4968/1/be/src/runtime/timestamp-value.cc
File be/src/runtime/timestamp-value.cc:

Line 101: void TimestampValue::Validate() {
Can you move the exception handling into DebugString()? This way it would be more obvious
to see there, that we handle the exception. And we would be less prone to adding new calls
to DebugString() that are not protected by Validate.


PS1, Line 103: 	  
Nit: tab. git-clang-format should fix this.


-- 
To view, visit http://gerrit.cloudera.org:8080/4968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message