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

minor: Move file compression #14555

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

logan-keede
Copy link
Contributor

@logan-keede logan-keede commented Feb 8, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Feb 8, 2025
@logan-keede
Copy link
Contributor Author

can I get some help with wasm ci? I have hit a wall there.

@logan-keede
Copy link
Contributor Author

cc @alamb

@alamb
Copy link
Contributor

alamb commented Feb 8, 2025

can I get some help with wasm ci? I have hit a wall there.

Looks like that has now been resolved, but there are some other issues that remain

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 8, 2025
@logan-keede
Copy link
Contributor Author

apologies, there were some major fumbles, this should not have taken 11+ commits,
I can open a separate PR if this would look too ugly.

@alamb
Copy link
Contributor

alamb commented Feb 8, 2025

apologies, there were some major fumbles, this should not have taken 11+ commits, I can open a separate PR if this would look too ugly.

No worries at all - when we merge to main everything in each PR is squashed into a single commit.

@logan-keede
Copy link
Contributor Author

No worries at all - when we merge to main everything in each PR is squashed into a single commit.

Thanks. I know but still, it feels off.

This is ready for review, let me know if you want some addition or changes.
There should be some scope of dependency minimisation, but I think it will be better/easier to do that after we have moved a bigger chunk of datasource.

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.

Thanks @logan-keede -- this is looking great. I had a few questions about feature flags but otherwise this is ready to go from my perpective

Thanks again for continuing to push on this

@@ -43,7 +43,7 @@ array_expressions = ["nested_expressions"]
# Used to enable the avro format
avro = ["apache-avro", "num-traits", "datafusion-common/avro"]
backtrace = ["datafusion-common/backtrace"]
compression = ["xz2", "bzip2", "flate2", "zstd", "async-compression", "tokio-util"]
compression = ["xz2", "bzip2", "flate2", "zstd"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised this doesn't need to activate the compression feature in datafusion-catalog-listing as well 🤔

@@ -43,6 +43,7 @@ chrono = { workspace = true, optional = true }
clap = { version = "4.5.16", features = ["derive", "env"] }
datafusion = { workspace = true, default-features = true, features = ["avro"] }
datafusion-catalog = { workspace = true, default-features = true }
datafusion-catalog-listing = { workspace = true, features = ["compression"] }
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 this should be necessary -- all the required featrues should be able to be activated via the datafusion crate.

Maybe you can just add compression as a feature abovez

datafusion = { workspace = true, default-features = true, features = ["avro"] }

@github-actions github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Feb 9, 2025
@logan-keede logan-keede requested a review from alamb February 9, 2025 17:30
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.

2 participants