Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16143 )

Change subject: IMPALA-9741: Support querying Iceberg table by impala
......................................................................


Patch Set 20:

(3 comments)

I am close to a +1/+2 once you've addressed Zoltan's comments.

I think there is a problem with the localhost hardcoded in the .json and .avro 
test data files though that will prevent data being loaded on many 
configurations. We could fix it if we replaced hdfs://localhost:20500/ with 
$DEFAULT_FS, but I'm not sure how best to do that with avro.

I'm going to run a precommit dry run to confirm that the dockerised tests will 
fail.

http://gerrit.cloudera.org:8080/#/c/16143/20/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java:

http://gerrit.cloudera.org:8080/#/c/16143/20/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@59
PS20, Line 59: icebergConjunts_
icebergConjuncts_


http://gerrit.cloudera.org:8080/#/c/16143/20/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@60
PS20, Line 60: icebergPredicate_
icebergPredicates_


http://gerrit.cloudera.org:8080/#/c/16143/20/testdata/data/iceberg_test/iceberg_non_partitioned/metadata/v1.metadata.json
File 
testdata/data/iceberg_test/iceberg_non_partitioned/metadata/v1.metadata.json:

PS20:
So there's a problem with these test tables. They have hdfs://localhost:20500 
hardcoded in the json and also in the avro files.

This needs to be substituted with $DEFAULT_FS somehow for it to work on 
alternative filesystems. We support running tests on non-hdfs filesystems and 
with alternative namenode hostnames.

This patch won't pass the precommit tests because the dockerized build use a 
different default fs



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I856cfee4f3397d1a89cf17650e8d4fbfe1f2b006
Gerrit-Change-Number: 16143
Gerrit-PatchSet: 20
Gerrit-Owner: wangsheng <[email protected]>
Gerrit-Reviewer: Anonymous Coward (606)
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Reviewer: wangsheng <[email protected]>
Gerrit-Comment-Date: Thu, 27 Aug 2020 23:13:38 +0000
Gerrit-HasComments: Yes

Reply via email to