You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2023/02/27 09:12:49 UTC
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19547
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
IMPALA-11950: Planner change for Iceberg metadata querying
This commit extends the planner with IcebergMetadataScanNode, this node
is only part of the query plan. At this point it is mostly a
placeholder, will be filled with more data when writing the backend
part.
To avoid executing the plan there is a hardcoded condition, it is after
the explain part, so the change remains testable with EXPLAIN queries.
Testing:
- Added planner tests
Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
---
M fe/src/main/java/org/apache/impala/analysis/Path.java
M fe/src/main/java/org/apache/impala/analysis/TableName.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
A fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
M fe/src/main/java/org/apache/impala/planner/ScanNode.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
10 files changed, 169 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/19547/1
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12808/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 09:13:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12451/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Feb 2023 09:33:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9248/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Apr 2023 09:04:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Apr 2023 09:04:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/Path.java
File fe/src/main/java/org/apache/impala/analysis/Path.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/Path.java@510
PS1, Line 510: org.apache.hadoop.hive.metastore.api.Table msTable = rootTable_.getMetaStoreTable();
Do you use this 'msTable' anywhere?
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@332
PS1, Line 332: RANDOM
Isn't DataPartition.UNPARTITIONED is more suitable for metadata table scan nodes?
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@73
PS1, Line 73:
nit: trailing spaces
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2223
PS1, Line 2223: Metadta
typo
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1288
PS1, Line 1288: metadtat
nit: typo
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test:
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@1
PS1, Line 1: explain SELECT * FROM functional_parquet.iceberg_alltypes_part_orc.history
Just a general comment to have coverage for most of the metadata tables here.
Additionally, experimenting with nested columns and how they are actually represented as tuple/slotdescriptors and how they affect the row-size would be also beneficial here, in my opinion.
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@6
PS1, Line 6: row-size=33B
It might be a valid use case to do some projection of the columns and check if the row-size decreases accordingly.
Additionally, if we add the same column multiple times in the select list, the row-size shouldn't grow as we can do a single read for the redundant columns.
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@7
PS1, Line 7: ---- DISTRIBUTEDPLAN
This line just made me wondering if it makes any sense to run a metadata query scan in a distributed way. I guess the whole query would be executed on one node, right?
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@29
PS1, Line 29: row-size=0B
Is it correct that the row-size for the scans here are zero while the join has a row-size of 66? I'd expect that the scans would have a row size of 33, otherwise there won't be any space in the row batch to populate the scan results to.
I'd check what slots are materialized on these nodes.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 14:35:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
IMPALA-11950: Planner change for Iceberg metadata querying
This commit extends the planner with IcebergMetadataScanNode which will
be used to scan Iceberg metadata tables (IMPALA-10947). The scan node is
only implemented on the frontend side in this patch, the backend part
will be developed in IMPALA-11996.
To avoid executing the plan there is a hardcoded condition, it is after
the explain part, so the change remains testable with EXPLAIN queries.
Testing:
- Added planner tests
Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
---
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
A fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
M fe/src/main/java/org/apache/impala/planner/ScanNode.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
8 files changed, 251 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/19547/5
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5:
(1 comment)
Thank you for the reviews!
http://gerrit.cloudera.org:8080/#/c/19547/5/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/5/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@41
PS5, Line 41: computeStats(analyzer);
> Is a metadata table has such a thing as stats? Does it make sense to comput
This will compute the average row sizes as well, so the plan can have better values, even though the actual statistics will not be used.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Apr 2023 09:04:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9230/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 08:53:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
IMPALA-11950: Planner change for Iceberg metadata querying
This commit extends the planner with IcebergMetadataScanNode which will
be used to scan Iceberg metadata tables (IMPALA-10947). The scan node is
only implemented on the frontend side in this patch, the backend part
will be developed in IMPALA-11996.
To avoid executing the plan there is a hardcoded condition, it is after
the explain part, so the change remains testable with EXPLAIN queries.
Testing:
- Added planner tests
Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Reviewed-on: http://gerrit.cloudera.org:8080/19547
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
A fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
M fe/src/main/java/org/apache/impala/planner/ScanNode.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
8 files changed, 251 insertions(+), 13 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 1:
(17 comments)
Thank you for the reviews, updated the patch.
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@9
PS1, Line 9: , this
> Nit: should start a new sentence here.
Done
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@11
PS1, Line 11: writing the backend
: part
> Is it going to be in another patch or this one? If another one, can you ind
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/Path.java
File fe/src/main/java/org/apache/impala/analysis/Path.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/Path.java@510
PS1, Line 510: org.apache.hadoop.hive.metastore.api.Table msTable = rootTable_.getMetaStoreTable();
> Do you use this 'msTable' anywhere?
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java
File fe/src/main/java/org/apache/impala/analysis/TableName.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java@59
PS1, Line 59: Preconditions.checkNotNull(tbl);
> Is it allowed to be empty?
During candidate table creation this constructor is used, so empty table name is allowed here.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@52
PS1, Line 52: metadataTableName_ = tblRefPath.get(2);
> Can there be interference with for example CollectionTableRefs which may ha
This is only called when isIcebergMetadataTable() returns true, so only for iceberg tables and when the third part of the table name matches the Iceberg metadtat table name.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@332
PS1, Line 332: RANDOM
> Isn't DataPartition.UNPARTITIONED is more suitable for metadata table scan
It is, nice catch.
While I was testing I wanted to see some more exciting plans, this removes some exchange nodes.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@36
PS1, Line 36: desc_ = desc;
> Do we allow a null desc_? If not, we could have a precondition check.
I did not see that for HdfsScanNode/HbaseScanNode so I would keep it this way. I assume it cannot be null.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@47
PS1, Line 47: Preconditions.checkNotNull(desc_.getPath());
> If desc_ can be null (see comment on L36), we should also check that it is
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@73
PS1, Line 73:
> nit: trailing spaces
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2223
PS1, Line 2223: Metadta
> typo
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2217
PS1, Line 2217: t
> Nit: extra t.
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2220
PS1, Line 2220: t
> Nit: extra t.
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1288
PS1, Line 1288: metadtat
> nit: typo
Done
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test:
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@1
PS1, Line 1: explain SELECT * FROM functional_parquet.iceberg_alltypes_part_orc.history
> Just a general comment to have coverage for most of the metadata tables her
I added coverage on parser side, do you think it would be useful to have plans for all the tables as well?
Also, added a plan with a nested type.
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@6
PS1, Line 6: row-size=33B
> It might be a valid use case to do some projection of the columns and check
Added a new test with projection.
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@7
PS1, Line 7: ---- DISTRIBUTEDPLAN
> This line just made me wondering if it makes any sense to run a metadata qu
Yes, the fragment will be executed on one node. I don't think it makes sense at this point to split the scanning.
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@29
PS1, Line 29: row-size=0B
> Is it correct that the row-size for the scans here are zero while the join
Nice catch, this is the result of the planning, missed to calculate the stats for the nodes. Fixed it.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Mar 2023 16:47:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Apr 2023 14:18:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 4:
(5 comments)
Thank you for the review Daniel, updated the change.
http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-11950: Planner change for Iceberg metadata querying
> I wonder if we should add something about the changes in processing cost ca
That is a different change that appeared due to the rebase.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java
File fe/src/main/java/org/apache/impala/planner/ScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@368
PS4, Line 368: else
> Nit: no need for else because the THEN branch returns.
This isn't my change, I only removed some unused code I noticed while investigating but it looks like that code was removed as well in a different change.
I will keep this file as is for now.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@390
PS4, Line 390: reflect
> Nit: reflects.
This line appeared due to the rebase. I will keep this file as is for now
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@866
PS4, Line 866: enclosingTupleDescs
> This variable is not used. This part was added here because of a rebase, ri
Yeah, chaining the changes was not the best idea, I was hoping for better support from gerrit.
Anyway, removed the unused variable, it makes the codebase cleaner.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java@294
PS4, Line 294: that
> Remove 'that'?
Done
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 08:52:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
IMPALA-11950: Planner change for Iceberg metadata querying
This commit extends the planner with IcebergMetadataScanNode which will
be used to scan Iceberg metadata tables (IMPALA-10947). The scan node is
only implemented on the frontend side in this patch, the backend part
will be developed in IMPALA-11996.
To avoid executing the plan there is a hardcoded condition, it is after
the explain part, so the change remains testable with EXPLAIN queries.
Testing:
- Added planner tests
Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
---
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
A fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
M fe/src/main/java/org/apache/impala/planner/ScanNode.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
8 files changed, 248 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/19547/4
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-11950: Planner change for Iceberg metadata querying
I wonder if we should add something about the changes in processing cost calculation and scan parallelism?
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@413
PS4, Line 413: ==
Shouldn't it be !=? Do we skip appending info at the highest explain level?
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@937
PS4, Line 937: )
Nit: no need to enclose the first two conjuncts in parentheses.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java
File fe/src/main/java/org/apache/impala/planner/ScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@368
PS4, Line 368: else
Nit: no need for else because the THEN branch returns.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@390
PS4, Line 390: reflect
Nit: reflects.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@866
PS4, Line 866: enclosingTupleDescs
This variable is not used. This part was added here because of a rebase, right? We can remove this line now but if you think it's too unrelated we can do it in another patch too.
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java@294
PS4, Line 294: that
Remove 'that'?
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 12:35:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12787/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 12:19:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19547/5/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/5/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@41
PS5, Line 41: computeStats(analyzer);
Is a metadata table has such a thing as stats? Does it make sense to compute stats?
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test
File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test:
http://gerrit.cloudera.org:8080/#/c/19547/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-metadata-table-scan.test@1
PS1, Line 1: explain SELECT * FROM functional_parquet.iceberg_alltypes_part_orc.history
> I added coverage on parser side, do you think it would be useful to have pl
It's fine, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Apr 2023 11:42:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 09:05:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 14:07:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11950: Planner change for Iceberg metadata querying
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@9
PS1, Line 9: , this
Nit: should start a new sentence here.
http://gerrit.cloudera.org:8080/#/c/19547/1//COMMIT_MSG@11
PS1, Line 11: writing the backend
: part
Is it going to be in another patch or this one? If another one, can you indicate that?
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java
File fe/src/main/java/org/apache/impala/analysis/TableName.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/analysis/TableName.java@59
PS1, Line 59: Preconditions.checkNotNull(tbl);
Is it allowed to be empty?
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@52
PS1, Line 52: metadataTableName_ = tblRefPath.get(2);
Can there be interference with for example CollectionTableRefs which may have a path longer than 2? Or is this function only called if we know this is not the case?
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@36
PS1, Line 36: desc_ = desc;
Do we allow a null desc_? If not, we could have a precondition check.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/planner/IcebergMetadataScanNode.java@47
PS1, Line 47: Preconditions.checkNotNull(desc_.getPath());
If desc_ can be null (see comment on L36), we should also check that it is not null here.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2217
PS1, Line 2217: t
Nit: extra t.
http://gerrit.cloudera.org:8080/#/c/19547/1/fe/src/main/java/org/apache/impala/service/Frontend.java@2220
PS1, Line 2220: t
Nit: extra t.
Also, shouldn't it be "metadata table"?
--
To view, visit http://gerrit.cloudera.org:8080/19547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6
Gerrit-Change-Number: 19547
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Mar 2023 14:49:19 +0000
Gerrit-HasComments: Yes