Skip to content

Conversation

@deniskuzZ
Copy link
Member

@deniskuzZ deniskuzZ commented Jan 24, 2023

BaseUpdatePartitionSpec::addField is failing with “Cannot add duplicate partition field” for the re-added DATE type field.

existing.transform().equals(sourceTransform.second()) returns FALSE since the types are different:

existing.transform() = {Dates@11599} “year”
sourceTransform.second() = {Years@11603} “year”

Test

Schema SCHEMA = new Schema(
          required(1, "id", Types.IntegerType.get()),
          required(2, "data", Types.StringType.get()),
          optional(3, "year_field", Types.DateType.get()));

PartitionSpec SPEC = PartitionSpec.builderFor(SCHEMA).year("year_field").build();
table = create(SCHEMA, SPEC);

table.updateSpec()
    .removeField(Expressions.year("year_field"))
    .addField(Expressions.year("year_field"));

@github-actions github-actions bot added the core label Jan 24, 2023
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

@deniskuzZ Thanks for the contribution! when you get a chance could you update the PR description about what the incompatibility issue (or if there's a github issue a link to that) is and why this change is required?

@deniskuzZ
Copy link
Member Author

@amogh-jahagirdar, thanks for checking! updated PR description and added unit test

@zhangbutao
Copy link
Contributor

I also had a workaround for this issue, see #6482. Not sure which way is more reasonable.

@deniskuzZ
Copy link
Member Author

I also had a workaround for this issue, see #6482. Not sure which way is more reasonable.

I am ok with either solution, however, noticed a potentially no-go comment in your PR and decided to take the same approach as @Fokko in #6220.
Note this is a blocker for Hive Iceberg 1.1.0 migration.

@zhangbutao
Copy link
Contributor

I am ok with either solution, however, noticed a potentially no-go comment in your PR and decided to take the same approach as @Fokko in #6220. Note this is a blocker for Hive Iceberg 1.1.0 migration.

Understood. I am ok with either solution too. Hope to fix this as soon as possible to unblock the Hive-iceberg upgrading.

@deniskuzZ
Copy link
Member Author

@Fokko could you please take a look

@deniskuzZ
Copy link
Member Author

deniskuzZ commented Feb 2, 2023

hi @pvary, could you please help with the review?

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, I was traveling the last few days. I like this solution. It is until Iceberg 2.0.0 that we have to keep the lazy and non-lazy versions of initializing a PartitionSpec using a transform.

@deniskuzZ
Copy link
Member Author

Sorry for the late reply, I was traveling the last few days. I like this solution. It is until Iceberg 2.0.0 that we have to keep the lazy and non-lazy versions of initializing a PartitionSpec using a transform.

Thank you!

@Fokko Fokko merged commit efe7ea6 into apache:master Feb 2, 2023
InvisibleProgrammer pushed a commit to InvisibleProgrammer/iceberg that referenced this pull request Mar 10, 2023
krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants