-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Split out datafusion-substrait
and datafusion-proto
CI feature checks, increase coverage
#15156
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
datafusion-substrait
and datafusion-proto
CI feature checks
# | ||
# Ensure via `cargo check` that the crate can be built with a | ||
# subset of the features packages enabled. | ||
linux-datafusion-common-features: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff makes of hard to read but I just moved what jobs each command was run in -- the overall coverage is the same or better
run: cargo check --profile ci --all-targets --no-default-features -p datafusion-substrait | ||
- name: Check datafusion-substrait (physical) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this adds coverage of compiling datafusion-substrait with the available feature flags
rust-version: stable | ||
- name: Check datafusion-proto (no-default-features) | ||
run: cargo check --profile ci --all-targets --no-default-features -p datafusion-proto | ||
# fails due to https://github.com/apache/datafusion/issues/15157 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds coverage for the datafusion-proto crate, and in fact found a bug:
datafusion-substrait
and datafusion-proto
CI feature checksdatafusion-substrait
and datafusion-proto
CI feature checks, increase coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks very reasonable to me and a good improvement. I left a note in the issue you raised that we will want to follow up on the commented out check.
Thank you for the review @timsaucer ! |
datafusion-substrait
and datafusion-proto
CI feature checks, increase coveragedatafusion-substrait
and datafusion-proto
CI feature checks, increase coverage
datafusion-substrait
and datafusion-proto
CI feature checks, increase coveragedatafusion-substrait
and datafusion-proto
CI feature checks, increase coverage
Which issue does this PR close?
Rationale for this change
The coverage for feature flags needs to be improved, as explained on #15155
What changes are included in this PR?
Note I will make a follow on PR to add additional coverage for flags in
datafusion-functions
anddatafusion
but I am trying to keep this PR reasonably sizedAre these changes tested?
Are there any user-facing changes?