Qifan Chen has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17857 )

Change subject: IMPALA-10914: Consistently schedule scan ranges for Iceberg 
tables
......................................................................


Patch Set 2:

(4 comments)

Thanks a lot for addressing my comments! Looks good!

http://gerrit.cloudera.org:8080/#/c/17857/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17857/1//COMMIT_MSG@11
PS1, Line 11:  and the HDFS
            : block locations were not consistent across the reload
> When we retrieve the file status from HDFS we get block location informatio
In this case, if we sort the block locations, would it make things better?

With this patch, it is true that we can only access iceberg tables associated 
with a snapshot? That worries me a bit.


http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1268
PS1, Line 1268:           addSummary(response, "Updated table.");
> I used the same message that we use at L1079.
Done


http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1272
PS1, Line 1272: Updated table.
> I used the same message that we use at L1090.
Done


http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1295
PS1, Line 1295:     if (!needsToUpdateHms) {
              :       loadTableMetadata(tbl, newCatalogVersion, true, true, 
null, "ALTER Iceberg TABLE " +
              :           params.getAlter_type().name());
              :       catalog_.addVersionsForInflightEvents(false, tbl, 
newCatalogVersion);
              :       addTableToCatalogUpdate(tbl, wantMinimalResult, 
response.result);
              :       return false;
              :     }
> SET PARTITION SPEC doesn't use an Iceberg transaction and we don't need to
I see. Maybe add a comment to indicate it is not transactional and the work is 
to set partition specs.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb8216b37d350469b573dad7fcefdd0ee0599ed5
Gerrit-Change-Number: 17857
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Qifan Chen <[email protected]>
Gerrit-Reviewer: Tamas Mate <[email protected]>
Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Mon, 27 Sep 2021 14:38:49 +0000
Gerrit-HasComments: Yes

Reply via email to