Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201612021711.uB2HB7U3026456@ip-10-146-233-104.ec2.internal>
Date: Fri, 2 Dec 2016 17:11:07 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: anujphadke <aphadke@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2057=3A_Better_error_message_for_incorrect_avro_decimal_column_declaration=0A?=
In-Reply-To: <gerrit.1480415157000.Iad23706128223b6537d565471ef5d8faa91b0b5a@gerrit.cloudera.org>
References: <gerrit.1480415157000.Iad23706128223b6537d565471ef5d8faa91b0b5a@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 02 Dec 2016 17:11:15 -0000

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