impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3776: fix 'describe formatted' for Avro tables
Date Thu, 25 Aug 2016 20:38:16 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-3776: fix 'describe formatted' for Avro tables
......................................................................


Patch Set 2: Code-Review-2

(6 comments)

Thanks for the review. I added tests and will push them directly to impala-asf. Sometime later
I will abandon this change.

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

Line 7: IMPALA-3776: fix 'describe formatted' for Avro tables
> For both cases below, is our output consistent with the regular describe (w
After this change, yes. Both commands eventually will call getNonClusteringColumns() and getClusteringColumns()
in Table.java, although the code paths to there are very(!) different and should be unified
in a subsequent change.


Line 11: 'avro.schema.url' file. HIVE-6308 aimed to improve upon this, but for
> Instead of 'avro.schema'url' let's just say Avro Schema because there are o
Done


Line 21: 2) The avro schema contains a column, which is not present in the
> Did this second case also have a bug that was fixed, or did it already work
This was also broken.


Line 25: I don't know how to automatically test this, but I verified this
> I think you might be able to test case 1) as follows:
Thank you for the hint, that led me in the right direction and I was able to test both cases.
:)


http://gerrit.cloudera.org:8080/#/c/3474/2/fe/src/main/java/com/cloudera/impala/service/DescribeResultFactory.java
File fe/src/main/java/com/cloudera/impala/service/DescribeResultFactory.java:

Line 187:     // table.
> table which has already reconciled those differences.
Done


Line 189:     msTable.setPartitionKeys(Column.toFieldSchemas(table.getClusteringColumns()));
> Nice solution!
It was Bharath's idea. :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic640f18acf7a1731f34b22c50ebbb462dfee78bd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message