Skip to content

Conversation

@adriangb
Copy link
Contributor

@adriangb adriangb commented Oct 8, 2025

Ever since #16014 we've been passing duplicate information in. FileMeta is created entirely from PartitionedFile. This simplifies APIs and removes an unnecessary struct.

@github-actions github-actions bot added core Core DataFusion crate datasource Changes to the datasource crate labels Oct 8, 2025
@adriangb adriangb requested a review from alamb October 8, 2025 05:25
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

Good cleaning, thank you.

I saw there are some API changes, so added the api-change label.

@xudong963 xudong963 added the api change Changes the API exposed to users of the crate label Oct 8, 2025
@adriangb
Copy link
Contributor Author

adriangb commented Oct 8, 2025

Do you think it's worth adding this to the upgrade guide? I think it's quite straightforward and will impact few people. So maybe not worth the noise?

&self,
_partition_index: usize,
file_meta: FileMeta,
file: PartitionedFile,
Copy link
Contributor

Choose a reason for hiding this comment

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

1 nit: we probably can name it as partition_file to be the same format as other function params

file_meta: FileMeta,
_file: PartitionedFile,
) -> Result<FileOpenFuture> {
fn open(&self, file: PartitionedFile) -> Result<FileOpenFuture> {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

&self,
partition_index: usize,
file_meta: FileMeta,
file: PartitionedFile,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@adriangb
Copy link
Contributor Author

adriangb commented Oct 8, 2025

@comphead I've renamed all uses to partitioned_file: PartitionedFile

@adriangb adriangb added this pull request to the merge queue Oct 8, 2025
Merged via the queue into apache:main with commit 5472e5d Oct 8, 2025
10 of 11 checks passed
@adriangb
Copy link
Contributor Author

adriangb commented Oct 8, 2025

Hmm I tried to put this in the queue but it seems to have merged immediately? I think it was ready but would have liked to see CI run. Not sure if it was a glitch in the GitHub UI or what. Anwyay I'll fix any issues in main or revert if needed.

@adriangb adriangb deleted the clean-up-apis branch October 8, 2025 19:04
adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Oct 8, 2025
@alamb
Copy link
Contributor

alamb commented Oct 8, 2025

Hmm I tried to put this in the queue but it seems to have merged immediately? I think it was ready but would have liked to see CI run. Not sure if it was a glitch in the GitHub UI or what. Anwyay I'll fix any issues in main or revert if needed.

Yeah, we are working on the merge-queue but it doesn't actually re-run CI yet -- see reason here: #6880 (comment)

file_meta: FileMeta,
_file: PartitionedFile,
) -> Result<FileOpenFuture> {
fn open(&self, partitioned_file: PartitionedFile) -> Result<FileOpenFuture> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nice improvement I agree

adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 27, 2025
* clean up duplicate information in FileOpener trait

* remove unused struct

* remove more

* rename paramter

* more renames

* minimize diff

* fix renames

* fix renames

* more fixes
adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate core Core DataFusion crate datasource Changes to the datasource crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants