Skip to content

Reapply "Ignore sort_query_fuzzer_runner (#16462)" (#16470) #16485

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

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jun 20, 2025

This reverts commit a0eaf51.

Which issue does this PR close?

Rationale for this change

However, then the test failed again when we re-enabled it:
https://github.com/apache/datafusion/actions/runs/15780623790/job/44485106315

Let's revert the revert

What changes are included in this PR?

Re-disable the test on main while we sort out the root cause

Are these changes tested?

Are there any user-facing changes?

@alamb alamb marked this pull request as ready for review June 20, 2025 14:59
@github-actions github-actions bot added the core Core DataFusion crate label Jun 20, 2025
Copy link
Contributor

@blaginin blaginin left a comment

Choose a reason for hiding this comment

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

😀

@alamb alamb merged commit 7914624 into apache:main Jun 20, 2025
10 checks passed
@alamb
Copy link
Contributor Author

alamb commented Jun 20, 2025

Thanks @blaginin I am merging this in to get main back clean again

@adriangb
Copy link
Contributor

Oh boy. @alamb should we revert #15770 altogether then? Does this test not run until it's merged into main / why want's it flagged as part of that PR?

@blaginin
Copy link
Contributor

blaginin commented Jun 20, 2025

We could also set enable_dynamic_filter_pushdown to false by default?

@adriangb
Copy link
Contributor

We could also set enable_dynamic_filter_pushdown to false by default?

Does that solve the issue? It didn't seem to in the last thread we had on it. If it does yes I think that's better than reverting the PR. Maybe we should do that out of caution regardless.

@alamb
Copy link
Contributor Author

alamb commented Jun 20, 2025

Oh boy. @alamb should we revert #15770 altogether then? Does this test not run until it's merged into main / why want's it flagged as part of that PR?

I don't think we should revert the PR given there is a way to turn it off -- let's just keep debugging on main. I'll try and find time over the weekend / next week if it is still unsolved

@alamb alamb deleted the alamb/revert_soo_many branch June 20, 2025 16:03
@alamb
Copy link
Contributor Author

alamb commented Jun 20, 2025

Basically from what I can tell we don't really know what the root cause is

@ozankabak
Copy link
Contributor

I think it is a good idea to revert #15770 on a branch, run tests for a few rounds on CI and see if the PR is the offending PR. If so, we can then revert it on main as we search for the root cause inside the changes #15770, which will significantly narrow down the possible causes.

@adriangb
Copy link
Contributor

I'm trying to reproduce locally first by running it 100 times and recording failure rate. I just ran it and got 0 failures. Not sure if this is expected or not. I'll push a branch to CI and try to get it to fail -> commit the revert -> no failures.

@adriangb
Copy link
Contributor

Second local run I got 4/100 failures. What a flake.

@alamb
Copy link
Contributor Author

alamb commented Jun 20, 2025

Second local run I got 4/100 failures. What a flake.

😱

@adriangb
Copy link
Contributor

Results:
Total runs: 497
Failures: 1
Passes: 496
Failure rate: 0.20%

😭

@ozankabak
Copy link
Contributor

Is this before the suspect PR?

@adriangb
Copy link
Contributor

I think it was after and I had made some edits to try to locate the section of code producing the but. But the comment was just to make the point of how flakey this is. Let's continue discussion in #16452

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.

4 participants