Skip to content

HIVE-27689: Iceberg: Remove unsed iceberg property #4681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zhangbutao wants to merge 3 commits into from
Closed

HIVE-27689: Iceberg: Remove unsed iceberg property #4681

zhangbutao wants to merge 3 commits into from

Conversation

@zhangbutao
Copy link
Contributor

@zhangbutao zhangbutao commented Sep 11, 2023 •
edited
Loading

What changes were proposed in this pull request?

Why are the changes needed?

It makes me feel confuesd when reading the code about InputFormatConfig.OPERATION_TYPE_PREFIX. This property introduced by HIVE-26102 was originally used to indicate the operation type of a output table, e.g DELETE&UPDATE&MERGE.
But after HIVE-26202 and HIVE-26264, this property won't get actual table operation type and it only can be used to identify the output table.

We should remove this property from current code snippet as this will misunderstand the meaning of the code, and use a proper term to indicate this table is an output table.

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

@zhangbutao zhangbutao force-pushed the remove_unused_iceberg_property branch from fd68ed8 to c1aea2b Compare September 12, 2023 01:27
@zhangbutao zhangbutao force-pushed the remove_unused_iceberg_property branch from c1aea2b to 605319b Compare September 13, 2023 02:54
@zhangbutao zhangbutao force-pushed the remove_unused_iceberg_property branch from 605319b to 0ef22cb Compare September 13, 2023 06:13
@zhangbutao zhangbutao changed the title Remove unsed iceberg property HIVE-27689: Iceberg: Remove unsed iceberg property Sep 14, 2023
zhangbutao
zhangbutao commented Sep 14, 2023
View reviewed changes
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java Outdated
public static final String COPY_ON_WRITE = "copy-on-write";
public static final String MERGE_ON_READ = "merge-on-read";
public static final String STATS = "/stats/";
static final String WRITE_KEY = "HiveIcebergStorageHandler_write";
Copy link
Contributor Author

@zhangbutao zhangbutao Sep 14, 2023

Choose a reason for hiding this comment

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

Here i use this term to indicate the table is an output table and this term was previously removed by HIVE-26102. If you have any better suggestion, feel free to comment.

Copy link
Member

@deniskuzZ deniskuzZ Oct 12, 2023

Choose a reason for hiding this comment

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

how about simply iceberg.mr.output.table?

Copy link
Contributor Author

@zhangbutao zhangbutao Oct 12, 2023

Choose a reason for hiding this comment

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

Changed this line to static final String WRITE_KEY = "iceberg.mr.output.table";

@zhangbutao
Copy link
Contributor Author

zhangbutao commented Sep 28, 2023

@ayushtkn @deniskuzZ Would you mind taking a look? Thanks.

@deniskuzZ
Copy link
Member

deniskuzZ commented Oct 12, 2023

@ayushtkn @deniskuzZ Would you mind taking a look? Thanks.

I think we could still leverage iceberg.mr.operation.type for COW

@zhangbutao zhangbutao force-pushed the remove_unused_iceberg_property branch from 0ef22cb to a84386a Compare October 12, 2023 16:20
@zhangbutao
Copy link
Contributor Author

zhangbutao commented Oct 12, 2023

I think we could still leverage iceberg.mr.operation.type for COW

Sure, I didn't delete it InputFormatConfig.OPERATION_TYPE_PREFIX, just remove it from these code block to avoid misunderstanding.

@zhangbutao zhangbutao force-pushed the remove_unused_iceberg_property branch from a84386a to 19f3a91 Compare October 15, 2023 05:56
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 15, 2023

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 0 Code Smells

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

warning The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@zhangbutao zhangbutao requested a review from deniskuzZ October 18, 2023 15:02
@github-actions
Copy link

github-actions bot commented Dec 18, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the [email protected] list if the patch is in need of reviews.

@github-actions github-actions bot added the stale label Dec 18, 2023
@deniskuzZ deniskuzZ removed the stale label Dec 20, 2023
@github-actions
Copy link

github-actions bot commented Feb 19, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the [email protected] list if the patch is in need of reviews.

@github-actions github-actions bot added the stale label Feb 19, 2024
@github-actions github-actions bot closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deniskuzZ deniskuzZ Awaiting requested review from deniskuzZ

Assignees

No one assigned

Labels

stale tests passed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zhangbutao @deniskuzZ @asf-ci-hive