Skip to content

Core|ORC|Spark: Remove deprecated functionality #6274

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 1 commit into from
Dec 18, 2022
Merged

Core|ORC|Spark: Remove deprecated functionality #6274

rdblue merged 1 commit into from
Dec 18, 2022

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Nov 25, 2022 •
edited
Loading

fixes #6155

@nastra nastra force-pushed the deprecations-for-1.2.0 branch 2 times, most recently from 7eefa7f to d7ab222 Compare November 28, 2022 08:29
nastra
nastra commented Nov 28, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/ManifestEntry.java
* @deprecated since 1.0.0, will be removed in 1.1.0; use {@link #dataSequenceNumber()} instead.
*/
@Deprecated
Long sequenceNumber();
Copy link
Contributor Author

@nastra nastra Nov 28, 2022

Choose a reason for hiding this comment

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

@aokolnychyi could you please review all places that adjust sequenceNumber -> dataSequenceNumber?

@nastra nastra force-pushed the deprecations-for-1.2.0 branch from d7ab222 to 6a64322 Compare November 28, 2022 09:09
@nastra nastra requested a review from aokolnychyi November 28, 2022 09:10
@nastra nastra force-pushed the deprecations-for-1.2.0 branch from 6a64322 to 39a983f Compare November 28, 2022 11:35
@github-actions github-actions bot added the data label Nov 28, 2022
@nastra nastra mentioned this pull request Nov 28, 2022
Merged
@nastra nastra force-pushed the deprecations-for-1.2.0 branch from 39a983f to 6df8657 Compare November 28, 2022 12:28
rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/GenericManifestEntry.java
private Status status = Status.EXISTING;
private Long snapshotId = null;
private Long dataSequenceNumber = null;
private Long sequenceNumber = null;
Copy link
Contributor

@rdblue rdblue Nov 30, 2022

Choose a reason for hiding this comment

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

@aokolnychyi, can you take a look at these changes?

rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
*/
@Deprecated
@Override
public List<ManifestFile> apply(TableMetadata base) {
Copy link
Contributor

@rdblue rdblue Nov 30, 2022

Choose a reason for hiding this comment

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

Why remove these? It doesn't seem like a big deal to keep the versions that default the parent snapshot to current.

Copy link
Contributor Author

@nastra nastra Dec 1, 2022

Choose a reason for hiding this comment

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

This is code that changed this method in #4926. We then restored+deprecated the old API in #6146 for one minor release and are removing it here now.
Let me know if we'd want to keep those methods, then we can just drop the Deprecated annotations from those

rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/deletes/Deletes.java
* Predicate, DeleteCounter)}.
*/
@Deprecated
public static <T> CloseableIterable<T> filterDeleted(
Copy link
Contributor

@rdblue rdblue Nov 30, 2022

Choose a reason for hiding this comment

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

I guess this is okay? It doesn't seem bad to have a version that doesn't require a counter.

Copy link
Contributor Author

@nastra nastra Dec 1, 2022

Choose a reason for hiding this comment

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

This is functionality that was adjusted by #4588. We then re-introduced + deprecated the method in #6146 to not have breaking API changes. Also there's currently no place in the code that would use this, so it seems better to remove it?

rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
rdblue
rdblue reviewed Nov 30, 2022
View reviewed changes
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkParquetReader.java
.build(),
MetricsConfig.getDefault())) {
writer.addAll(rows);
try (ParquetWriter<InternalRow> writer =
Copy link
Contributor

@rdblue rdblue Nov 30, 2022

Choose a reason for hiding this comment

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

Why were these changes needed?

Copy link
Contributor Author

@nastra nastra Dec 1, 2022

Choose a reason for hiding this comment

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

this was using the deprecated ParquetWriteAdapter class, so I started changing code that uses this class so that we can eventually remove ParquetWriteAdapter

@nastra nastra force-pushed the deprecations-for-1.2.0 branch from 6df8657 to 623b7aa Compare December 1, 2022 08:27
@github-actions github-actions bot removed the AWS label Dec 1, 2022
@aokolnychyi
Copy link
Contributor

aokolnychyi commented Dec 1, 2022

I'll take a look at seq number related changes today.

aokolnychyi
aokolnychyi reviewed Dec 2, 2022
View reviewed changes
aokolnychyi
aokolnychyi reviewed Dec 2, 2022
View reviewed changes
aokolnychyi
aokolnychyi reviewed Dec 2, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/TableProperties.java
/**
* @deprecated will be removed in 0.14.0, the cardinality check is always performed starting from
* 0.13.0.
* @deprecated will be removed once Spark 3.1 support is dropped, the cardinality check is always
Copy link
Contributor

@aokolnychyi aokolnychyi Dec 2, 2022

Choose a reason for hiding this comment

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

Makes sense to me.

aokolnychyi
aokolnychyi approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@aokolnychyi aokolnychyi left a comment

Choose a reason for hiding this comment

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

LGTM, I'd just remove two comments that no longer apply. Thanks, @nastra!

@nastra nastra force-pushed the deprecations-for-1.2.0 branch from 623b7aa to 48a9ade Compare December 2, 2022 08:23
@nastra nastra requested a review from rdblue December 2, 2022 08:24
@nastra nastra force-pushed the deprecations-for-1.2.0 branch from 48a9ade to 735eaf8 Compare December 9, 2022 13:00
@rdblue rdblue merged commit 5a9eb3c into apache:master Dec 18, 2022
@rdblue
Copy link
Contributor

rdblue commented Dec 18, 2022

Thanks, @nastra!

@nastra nastra deleted the deprecations-for-1.2.0 branch December 19, 2022 06:49
@nastra nastra mentioned this pull request Feb 17, 2023
Closed
gaborkaszab pushed a commit to gaborkaszab/iceberg that referenced this pull request May 5, 2023
Core: Remove duplicate check for ManifestEntry.dataSequenceNumber()
bda695d 
When dropping ManifestEntry.sequenceNumber in apache#6274, a new assert was
added to TableTestBase.validateManifests(). However, there was a same
assert already. Dropping one of them.
@gaborkaszab gaborkaszab mentioned this pull request May 5, 2023
Merged
gaborkaszab pushed a commit to gaborkaszab/iceberg that referenced this pull request May 5, 2023
Core: Remove duplicate check for ManifestEntry.dataSequenceNumber()
2152fd6 
When dropping ManifestEntry.sequenceNumber in apache#6274, a new assert was
added to validateManifest() and validateDeleteManifest(). However,
there was a same assert already. Dropping one of them.
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

@aokolnychyi aokolnychyi aokolnychyi approved these changes

Assignees

No one assigned

Labels

core data ORC spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove API deprecations for 1.2.0

3 participants

@nastra @aokolnychyi @rdblue