Skip to content

API: Make the PartitionSpec less lazy #6220

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
Merged
rdblue merged 5 commits into from
Nov 20, 2022
Merged

API: Make the PartitionSpec less lazy #6220

rdblue merged 5 commits into from
Nov 20, 2022

Conversation

@Fokko
Copy link
Contributor

@Fokko Fokko commented Nov 18, 2022 •
edited
Loading

Debugging some regression that we have at Trino, this PR seems to fix a part of it.

It looks like we've deprecated this a bit too fast. Instead, when we read the PartitionSpec from the manifest, we should bind them to maintain compatibility with 1.0.0.

java.lang.UnsupportedOperationException: hash(value) is not supported on the base Bucket class
	at org.apache.iceberg.transforms.Bucket.hash(Bucket.java:90)
	at org.apache.iceberg.transforms.Bucket.apply(Bucket.java:99)
	at org.apache.iceberg.transforms.Bucket.apply(Bucket.java:38)
	at io.trino.plugin.iceberg.IcebergPageSink.applyTransform(IcebergPageSink.java:372)
	at io.trino.plugin.iceberg.IcebergPageSink.getPartitionData(IcebergPageSink.java:364)
	at io.trino.plugin.iceberg.IcebergPageSink.getWriterIndexes(IcebergPageSink.java:288)
	at io.trino.plugin.iceberg.IcebergPageSink.writePage(IcebergPageSink.java:215)
	at io.trino.plugin.iceberg.IcebergPageSink.doAppend(IcebergPageSink.java:210)
	at io.trino.plugin.iceberg.IcebergPageSink.appendPage(IcebergPageSink.java:161)

@Fokko Fokko changed the title API: Pass in the types API: Pass in the types when creating PartitionSpec Nov 18, 2022
@github-actions github-actions bot added the API label Nov 18, 2022
@Fokko Fokko changed the title API: Pass in the types when creating PartitionSpec API: Make the PartitionSpec less lazy Nov 18, 2022
@Fokko Fokko force-pushed the fd-fix-backward-compatibility-of-partition-spec-transform branch from 326b6c0 to c8b16a8 Compare November 18, 2022 16:02
@Fokko
Copy link
Contributor Author

Fokko commented Nov 18, 2022

Trino is now passing without changes on their side:
image

Fokko
Fokko commented Nov 19, 2022
View reviewed changes
api/src/main/java/org/apache/iceberg/transforms/ProjectionUtil.java
Transform<?, T> transform, String partitionName, BoundPredicate<?> pred) {
if (pred.term() instanceof BoundTransform
&& transform.equals(((BoundTransform<?, ?>) pred.term()).transform())) {
&& transform
Copy link
Contributor Author

@Fokko Fokko Nov 19, 2022

Choose a reason for hiding this comment

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

I had to change this to make testProjectionNames pass. In the case of hour transform, we'll be comparing transforms which is an instance of Timestamps with Hours. The toString() both produce hour.

Copy link
Contributor

@rdblue rdblue Nov 20, 2022

Choose a reason for hiding this comment

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

We should be able to roll this back once we deprecate the old apply method in favor of the bind approach. Then we don't need to keep two classes around for those.

@Fokko Fokko force-pushed the fd-fix-backward-compatibility-of-partition-spec-transform branch 3 times, most recently from 881315d to 6381d9d Compare November 19, 2022 22:02
@github-actions github-actions bot added the core label Nov 19, 2022
@Fokko Fokko force-pushed the fd-fix-backward-compatibility-of-partition-spec-transform branch from 6381d9d to 69b5a59 Compare November 19, 2022 22:07
@Fokko
Merge branch 'master' of github.com:apache/iceberg into fd-fix-backwa…
687e346 
…rd-compatibility-of-partition-spec-transform
rdblue
rdblue reviewed Nov 19, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java Outdated
Pair<Transform<?, ?>, Integer> sourceAndTransform =
Pair.of(sortField.transform(), sortField.sourceId());
Pair<String, Integer> sourceAndTransform =
Pair.of(sortField.transform().dedupName(), sortField.sourceId());
Copy link
Contributor

@rdblue rdblue Nov 19, 2022

Choose a reason for hiding this comment

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

I think this needs to be toString rather than dedupName. The dedupName method should not really be public and shouldn't be used for cases like this. We want the actual transform here.

Copy link
Contributor Author

@Fokko Fokko Nov 19, 2022

Choose a reason for hiding this comment

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

Got it, I've updated the PR! 👍🏻

@Fokko Fokko force-pushed the fd-fix-backward-compatibility-of-partition-spec-transform branch from 180c142 to f567fa0 Compare November 20, 2022 08:50
@rdblue rdblue added this to the Iceberg 1.1.0 Release milestone Nov 20, 2022
@rdblue rdblue merged commit 2918735 into apache:master Nov 20, 2022
@rdblue
Copy link
Contributor

rdblue commented Nov 20, 2022

Thanks, @Fokko! Good find.

@zhangbutao zhangbutao mentioned this pull request Dec 22, 2022
Closed
@deniskuzZ deniskuzZ mentioned this pull request Jan 30, 2023
Merged
@s-akhtar-baig s-akhtar-baig mentioned this pull request Aug 2, 2023
Merged
@Fokko Fokko deleted the fd-fix-backward-compatibility-of-partition-spec-transform branch December 3, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdblue rdblue rdblue approved these changes

Assignees

No one assigned

Labels

API core

Projects

None yet

Milestone

Iceberg 1.1.0 Release

Development

Successfully merging this pull request may close these issues.

2 participants

@Fokko @rdblue