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

Remove unbounded_input from FileSinkOptions #8605

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

devinjdangelo
Copy link
Contributor

Which issue does this PR close?

None, but related to #8604

Rationale for this change

Now that writing unbounded (streaming) sources is supported via StreamWrite, other DataSink impls do not need to worry about yielding on unbounded input plans.

What changes are included in this PR?

Removes unbounded_input checks in FileSinkConfig

Are these changes tested?

Yes, by existing tests

Are there any user-facing changes?

No

@github-actions github-actions bot added the core Core DataFusion crate label Dec 20, 2023
@@ -28,6 +28,7 @@ fn main() -> Result<(), String> {
let descriptor_path = proto_dir.join("proto_descriptor.bin");

prost_build::Config::new()
.protoc_arg("--experimental_allow_proto3_optional")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be able to remove this after #8602 is fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed merged main, and confirmed that 8602 is fixed for me as well

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you (again) @devinjdangelo

@metesynnada / @ozankabak can you please also review this PR to make sure we are not removing something that is important for your usecase?

Copy link
Contributor

@metesynnada metesynnada left a comment

Choose a reason for hiding this comment

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

LGTM.

@alamb alamb merged commit 96c5b8a into apache:main Dec 21, 2023
22 checks passed
@alamb
Copy link
Contributor

alamb commented Dec 21, 2023

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants