[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-09 Thread Internal Jenkins (Code Review) 
Internal Jenkins has posted comments on this change.

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


Patch Set 5: Verified+1

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-09 Thread Bharath Vissapragada (Code Review) 
Bharath Vissapragada has posted comments on this change.

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


Patch Set 5: Code-Review+2

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-06 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

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


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-06 Thread anujphadke (Code Review) 
anujphadke has uploaded a new patch set (#4).

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

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

Adding a better error message when logical type is specified at a wrong
level or is not not specified in an avro decimal column declaration.

Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
---
M fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java
1 file changed, 24 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/5255/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-05 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(2 comments)

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

Line 139: "Unsupported logicalType: " + logicalType));
Should also include something like "for column '%s' with type BYTES" so users 
can understand better what happened.


PS3, Line 180: , String logicalType
Unused argument


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-04 Thread anujphadke (Code Review) 
anujphadke 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.
Done


Line 178: if (logicalType.equalsIgnoreCase("decimal")) {
> I think this should be checked before calling getDecimalType(). It makes mo
Done


Line 193:   "Unsupported logicalType");
> Should include the type name in the exception method so that users can unde
Previous comment -
  3. getDecimalType() should now either return a valid type or throw an 
exception.

Done.
Had to move the exception in an else statement 
since 
if (logicalType.equalsIgnoreCase("decimal")) 
this check was moved outside.


-- 
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 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-04 Thread anujphadke (Code Review) 
anujphadke has uploaded a new patch set (#3).

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

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

Adding a better error message when logical type is specified at a wrong
level or is not not specified in an avro decimal column declaration.

Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
---
M fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java
1 file changed, 24 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/5255/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-02 Thread Tim Armstrong (Code Review) 
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 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-02 Thread anujphadke (Code Review) 
anujphadke has posted comments on this change.

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


Patch Set 2:

1. check if a logical type is present, and throw an exception if it's non-NULL 
(since BYTES requires a logical type) - Moved the the check before calling 
getDecimalType

  2. call getDecimalType(schema, logicalType) - Done 

  3. getDecimalType() should now either return a valid type or throw an 
exception. 
   Done.

-- 
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 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-12-02 Thread anujphadke (Code Review) 
anujphadke has uploaded a new patch set (#2).

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

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

Adding a better error message when logical type is specified at a wrong
level or is not not specified in an avro decimal column declaration.

Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
---
M fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java
1 file changed, 12 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/5255/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-11-29 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(1 comment)

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

Line 131: String logicalType = schema.getProp("logicalType");
The control-flow here is unnecessarily convoluted (it was before your patch 
with returning null and the fall-through of the case). I think we should clean 
it up instead of adding to the complexity. It would make more sense to:

  1. check if a logical type is present, and throw an exception if it's 
non-NULL (since BYTES requires a logical type)
  2. call getDecimalType(schema, logicalType)
  3. getDecimalType() should now either return a valid type or throw an 
exception.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2057: Better error message for incorrect avro decimal column declaration

2016-11-29 Thread anujphadke (Code Review) 
anujphadke has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5255

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

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

Adding a better error message when logical type is specified at a wrong
level or is not not specified in an avro decimal column declaration.

Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
---
M fe/src/main/java/org/apache/impala/util/AvroSchemaParser.java
1 file changed, 6 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/5255/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iad23706128223b6537d565471ef5d8faa91b0b5a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke