Skip to content

Conversation

@wypoon
Copy link
Contributor

@wypoon wypoon commented Apr 8, 2024

This is an update of #6056.
In the original PR, some APIs were changed which are now checked by revapi. Instead of changing the APIs, I have retained but deprecated the original versions of the methods and added new versions with default implementations.
I have also ported the change in ColumnarBatchReader to Spark 3.4 and 3.5.

\ org.apache.iceberg.Schema, org.apache.iceberg.TableScanContext)"
justification: "Removing deprecated code"
"1.5.0":
org.apache.iceberg:iceberg-parquet:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can break existing APIs without deprecating them first. Please check https://iceberg.apache.org/contribute/#adding-new-functionality-without-breaking-apis

Copy link
Contributor Author

@wypoon wypoon Apr 9, 2024

Choose a reason for hiding this comment

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

@nastra thanks for the pointer. I have retained the old versions of the methods but deprecated them.

@wypoon wypoon force-pushed the parquet_use_getRowIndexOffset branch from a1bcf21 to a640047 Compare April 9, 2024 19:21
@wypoon wypoon changed the title [draft] Use Parquet's getRowIndexOffset support instead of calculating it Parquet: Use native getRowIndexOffset support instead of calculating it Apr 9, 2024
@wypoon
Copy link
Contributor Author

wypoon commented Apr 9, 2024

@flyrain I have updated your PR. Please review. cc @chenjunjiedada @Fokko.
One thing I'd like to add is a test that would fail with Parquet 1.12.3 (which has the bug PARQUET-2161) but would pass now. Suggestions on such a test would be welcome.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 26, 2024
@github-actions
Copy link

github-actions bot commented Nov 3, 2024

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@huaxingao
Copy link
Contributor

@wypoon Would you like to reopen this PR?

@wypoon
Copy link
Contributor Author

wypoon commented Nov 11, 2024

@huaxingao yes, I'll be happy to reopen it. The versions in the deprecation comments need to be updated. I'll update the PR soon.

@wypoon
Copy link
Contributor Author

wypoon commented Nov 12, 2024

I don't see how to reopen this. I have opened a new PR, #11520.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants