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: enable DF's nested_expressions feature by in datafusion-substrait tests to make them pass #13857

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Blizzara
Copy link
Contributor

Which issue does this PR close?

Closes #13854

Rationale for this change

https://github.com/apache/datafusion/pull/13594/files#diff-d195b6dfaaed44b9828e145dc50c075c77b7faf8f44783ca5e6eda2c617af928L39 removed DF default features from the substrait crate. That broke some tests that depended on stuff coming from nested_expressions feature.

Interestingly, the tests still passed in CI but failed when run locally - maybe due to CI running a whole bunch of tests at once and thus compiling DF with different set of features, or something?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

No - but in the original PR that caused the issue, that might be a breaking change to some consumer if someone has disabled default_features for DF but things have been working still because datafusion-substrait has been pulling those features in anyways. Not sure if that can happen - I checked our internal stuff and that seems to work fine against a recent commit on main.

@Blizzara
Copy link
Contributor Author

Fyi @alamb

@alamb
Copy link
Contributor

alamb commented Dec 20, 2024

Interestingly, the tests still passed in CI but failed when run locally - maybe due to CI running a whole bunch of tests at once and thus compiling DF with different set of features, or something?

Yeah I think they are run like this
https://github.com/apache/datafusion/blob/079f6219fed8eb8812b028257c031dfdfba96cb4/.github/workflows/rust.yml#L160C1-L161C1

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 @Blizzara -- I verified this works locally for me too.

It is annoying that CI didn't catch this but I also think it might be too much effort / overhead to add a specific ci check for each sub package.If it happens again we can maybe add some more tests

@alamb alamb merged commit d7aeb1a into apache:main Dec 20, 2024
26 checks passed
@alamb
Copy link
Contributor

alamb commented Dec 20, 2024

Thank you very much @Blizzara -- great 🕵️

I also filed a follow up ticket to make the error message more helpful

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

Successfully merging this pull request may close these issues.

substrait_integration integration tests are failing
2 participants