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-4387: validate decimal type in Avro file schema
Date Fri, 28 Oct 2016 23:52:19 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4387: validate decimal type in Avro file schema
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4876/2//COMMIT_MSG
Commit Message:

PS2, Line 16: scans
> nit: wrap at 80 chars.
Done


http://gerrit.cloudera.org:8080/#/c/4876/2/be/src/util/avro-util.cc
File be/src/util/avro-util.cc:

Line 109:     case AVRO_BYTES:
> maybe add a // fallthrough comment at the end of the line?
It doesn't seem necessary when the case labels are adjacent like this, we don't do it in other
parts of the codebase.


Line 139:       return Status::OK();
> Wouldn't it be better to return a non-OK status in a release build? If not,
Good point, this value comes from the Avro library so we don't have any guarantee it is a
supported type. Fixed it.


http://gerrit.cloudera.org:8080/#/c/4876/2/be/src/util/avro-util.h
File be/src/util/avro-util.h:

PS2, Line 84: name
> column_name?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I25a326ee2220bc14d3b5f887dc288b4adf859cfc
Gerrit-PatchSet: 2
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-Reviewer: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message