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-2057: Better error message for incorrect avro decimal column declaration
Date Fri, 02 Dec 2016 17:11:07 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2057: Better error message for incorrect avro decimal column declaration
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5255/2/fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java
File fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java:

Line 168:    * Attempts to parse decimal type information from the Avro schema, returning
This comment is now out-of-sync with the method.


Line 178:     if (logicalType.equalsIgnoreCase("decimal")) {
I think this should be checked before calling getDecimalType(). It makes more sense to me
for the caller to check the logical type and then call the appropriate function. E.g. later
on it could evolve to:

if (logicalType.equals("decimal")) {
  type = getDecimalType(schema);
} else if (logicalType.equals("another_type")) {
  type = getAnotherType(schema);
}

etc.


Line 193:           "Unsupported logicalType");
Should include the type name in the exception method so that users can understand what went
wrong.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <aphadke@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message