Skip to content
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.

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

fix: use table config target file size, expose target_file_size in python #2811

Merged

Conversation

ion-elgreco
Copy link
Collaborator

@ion-elgreco ion-elgreco commented Aug 21, 2024

Description

If the target_file_size is None but we have a snapshot, then we should take it from the table config, otherwise we try to fetch it from the provided configuration since the setting might be in there.

If you pass delta.targetFileSize to the configuration, this will be used even on first writes. Additionally target_file_size can be passed to the rust writer, which takes precedence over any table config that has been set

Related issues

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Aug 21, 2024
@ion-elgreco ion-elgreco force-pushed the fix/use_table_target_file_size branch from 1893da3 to 42f73b1 Compare August 21, 2024 20:05
@ion-elgreco ion-elgreco marked this pull request as draft August 21, 2024 20:30
auto-merge was automatically disabled August 21, 2024 20:30

Pull request was converted to draft

@github-actions github-actions bot added the binding/python Issues for the Python package label Aug 22, 2024
@ion-elgreco ion-elgreco marked this pull request as ready for review August 22, 2024 06:00
@ion-elgreco ion-elgreco changed the title fix: use table config target file size fix: use table config target file size, expose target_file_size in python Aug 22, 2024
@ion-elgreco ion-elgreco force-pushed the fix/use_table_target_file_size branch from aee6132 to 0388ab6 Compare August 22, 2024 20:00
/// Get the target_file_size from the table configuration in the sates
/// If table_config does not exist (only can occur in the first write action) it takes
/// the configuration that was passed to the writerBuilder.
pub fn get_target_file_size(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should become part of our public API, so I'm going to slap a crate level visibility on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Woops, my bad, should have been internal indeed

@rtyler rtyler force-pushed the fix/use_table_target_file_size branch from 0388ab6 to 452f8a3 Compare August 30, 2024 18:59
rtyler
rtyler previously approved these changes Aug 30, 2024
@ion-elgreco ion-elgreco added this pull request to the merge queue Sep 3, 2024
Merged via the queue into delta-io:main with commit a6cb348 Sep 3, 2024
21 checks passed
@ion-elgreco ion-elgreco deleted the fix/use_table_target_file_size branch September 3, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/python Issues for the Python package binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose target_file_size in python side for WriterProperties
2 participants