Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20753 )

Change subject: IMPALA-12597: Basic Equality delete read support for Iceberg 
tables
......................................................................


Patch Set 8:

(3 comments)

I only had one comment that might require some additional change: the one about 
'rewrite-iceberg-metadata.py'

Otherwise I'm ready to +2 it.

http://gerrit.cloudera.org:8080/#/c/20753/8/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:

http://gerrit.cloudera.org:8080/#/c/20753/8/tests/query_test/test_iceberg.py@1264
PS8, Line 1264:     SRC_DIR = os.path.join(os.environ['IMPALA_HOME'],
              :         "testdata/data/iceberg_test/hadoop_catalog/ice/"
              :         "iceberg_v2_delete_different_equality_ids")
              :     DST_DIR = 
"/test-warehouse/iceberg_test/hadoop_catalog/ice/" \
              :         "iceberg_v2_delete_different_equality_ids"
              :
              :     try:
              :       self.filesystem_client.make_dir(DST_DIR, permission=777)
              :
              :       
self.filesystem_client.copy_from_local(os.path.join(SRC_DIR, "data"), DST_DIR)
              :       
self.filesystem_client.copy_from_local(os.path.join(SRC_DIR, "metadata"), 
DST_DIR)
> Good Idea. I gave this a try, however I realized that for this particular t
I see, thanks for giving it a try.

Maybe invoking testdata/bin/rewrite-iceberg-metadata.py could be added, so this 
test could run in non-HDFS environments as well.


http://gerrit.cloudera.org:8080/#/c/20753/8/tests/query_test/test_iceberg.py@1278
PS8, Line 1278: external
> If I create it as a non-external table I get an error saying that the table
I see.
I also find it strange that create_iceberg_table_from_directory() creates 
external tables, especially that right after table creation it sets table 
property 'external.table.purge'='True'


http://gerrit.cloudera.org:8080/#/c/20753/8/tests/query_test/test_iceberg.py@1290
PS8, Line 1290:         assert "[1, 2]" in str(err)
              :         assert "[1]" in str(err)
> These are the mismatching equality ID lists. Added a comment for more clari
Thanks!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2053e6f321c69f1c82059a84a5d99aeaa9814cad
Gerrit-Change-Number: 20753
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <[email protected]>
Gerrit-Reviewer: Andrew Sherman <[email protected]>
Gerrit-Reviewer: Daniel Becker <[email protected]>
Gerrit-Reviewer: Gabor Kaszab <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Tamas Mate <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Mon, 18 Dec 2023 15:30:02 +0000
Gerrit-HasComments: Yes

Reply via email to