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 <apha...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-HasComments: Yes
