Skip to content

HIVE-26832: Implement SHOW PARTITIONS for Iceberg Tables - #4346

Merged
deniskuzZ merged 7 commits into
apache:masterfrom
simhadri-g:HIVE-26832
Jun 24, 2023
Merged

HIVE-26832: Implement SHOW PARTITIONS for Iceberg Tables#4346
deniskuzZ merged 7 commits into
apache:masterfrom
simhadri-g:HIVE-26832

Conversation

@simhadri-g

@simhadri-g simhadri-g commented May 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

SHOW PARTITIONS support for iceberg tables.

Why are the changes needed?

For Hive partition table , the show partition command outputs the following:

+---------------------+
|      partition      |
+---------------------+
| d_part=10/e_part=5  |
| d_part=2/e_part=5   |
| d_part=3/e_part=4   |
+---------------------+

In this PR, we are implementing the same feature for hive iceberg tables:

+--------------------------------+
|      partition                 |
+--------------------------------+
| Spec-id=0/d_part=10/e_part=5   |
| Spec-id=0/d_part=2/e_part=5    |
| Spec-id=0/d_part=3/e_part=4    |
+--------------------------------+

With partition evolution, the output will be per spec id:

+--------------------------------+
|      partition                 |
+--------------------------------+
| Spec-id=0/d_part=10/e_part=5   |
| Spec-id=0/d_part=2/e_part=5.   |
| Spec-id=0/d_part=3/e_part=4    |
| Spec-id=1/c=2                  |
| Spec-id=1/c=3                  |
| Spec-id=1/c=4                  |
| Spec-id=1/c=5                  |
+--------------------------------+

Does this PR introduce any user-facing change?

How was this patch tested?

Q test

@github-actions
github-actions Bot requested a review from miklosgergely May 20, 2023 20:30
aturoczy
aturoczy suggested changes May 22, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java Outdated
okumin
okumin reviewed May 22, 2023
View reviewed changes
Comment thread ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/show/ShowPartitionsOperation.java Outdated
throw new HiveException(ErrorMsg.TABLE_NOT_PARTITIONED, desc.getTabName());
boolean isIcebergTable = isIcebergTable(tbl);
if (!(tbl.isPartitioned() || isIcebergTable)) {
throw new HiveException(ErrorMsg.TABLE_NOT_PARTITIONED, desc.getTabName());

@okumin okumin May 22, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we generalize this part rather than hardcoding ICEBERG? I guess this is useful for another storage handler.

@aturoczy aturoczy May 22, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point

@okumin okumin May 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha. We've been developing a tailored storage with some similar concepts to Iceberg for 10 years. We really like recent changes for Iceberg because it would give us a chance to move our hacks to standard APIs of storage handlers. Let me check newly added APIs and propose better generalization.

@aturoczy aturoczy May 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha. We've been developing a tailored storage with some similar concepts to Iceberg for 10 years. We really like recent changes for Iceberg because it would give us a chance to move our hacks to standard APIs of storage handlers. Let me check newly added APIs and propose better generalization.

Yes, Iceberg has a very elegant API and solutions. I like it, also the concept removes lot of hacks from the hive side as well.

@simhadri-g simhadri-g May 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we generalize this part rather than hardcoding ICEBERG? I guess this is useful for another storage handler.

Done.

@deniskuzZ deniskuzZ May 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this block is to handle cases when you call show partitions on non-partitioned table. no need to protect if there is no partition support, especially when you added the default impl for showPartitions in storage handler

deniskuzZ
deniskuzZ reviewed Jun 16, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java Outdated
deniskuzZ
deniskuzZ reviewed Jun 16, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java Outdated
deniskuzZ
deniskuzZ reviewed Jun 16, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java Outdated
deniskuzZ
deniskuzZ reviewed Jun 16, 2023
View reviewed changes
Comment thread ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageHandler.java
deniskuzZ
deniskuzZ reviewed Jun 16, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/test/results/positive/show_partitions_test.q.out Outdated
deniskuzZ
deniskuzZ reviewed Jun 23, 2023
View reviewed changes
Comment thread iceberg/iceberg-handler/src/test/results/positive/show_partitions_test.q.out Outdated
deniskuzZ
deniskuzZ approved these changes Jun 23, 2023
View reviewed changes

@deniskuzZ deniskuzZ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @simhadri-g

@sonarqubecloud

sonarqubecloud Bot commented Jun 23, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@okumin okumin okumin left review comments
@deniskuzZ deniskuzZ deniskuzZ approved these changes
@miklosgergely miklosgergely Awaiting requested review from miklosgergely
+1 more reviewer
@aturoczy aturoczy aturoczy requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tests passed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@simhadri-g @okumin @deniskuzZ @aturoczy @kgyrtkirk