You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/06/22 13:06:40 UTC
[PR] Alamb/revert fix [datafusion]
alamb opened a new pull request, #16501:
URL: https://github.com/apache/datafusion/pull/16501
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
- @AdamGS removed some of the code here https://github.com/apache/datafusion/pull/16465
- However, the test kept failing
- @adriangb fixed what we think is the real issue here: https://github.com/apache/datafusion/pull/16491
## What changes are included in this PR?
Let's restore the removed code
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] restore topk pre-filtering of batches and make sort query fuzzer less sensitive to expected non determinism [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #16501:
URL: https://github.com/apache/datafusion/pull/16501
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] restore topk pre-filtering of batches and make sort query fuzzer less sensitive to expected non determinism [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3025439277
Amazing -- thank you @adriangb and @Dandandan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2996918815
Would love to give a hand with that, I have some thoughts I can try and put into a preliminary PR.
It also seems like Datafusion is going to have more of this shared state that's sensetive to how event interleave, and it might be worth it to make a larger effort to enable (more) deterministing simulation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2994226988
Sadly the 1200 run still reports failures ðŸ˜
I feel like @AdamGS 's original intuition that it's something about sort stabilit with nulls is correct. I'll see if I can find a fix...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2994232687
Thanks @adriangb !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3016903566
I think a relative simple solution might be only checking columns from the generated sort expressions to be have equal values (i.e. unstable sorting).
FYI: [2010YOUY01](https://github.com/2010YOUY01) what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3024609927
Since there seems to be agreement on the path forward I pushed 514ab74e0 which I think achieves the goal by simply changing `SELECT *` to `SELECT <same columns we're doing ordering by>`. Then we can continue to assert that the batches are equal, etc. I considered a more complex system where we keep track of the ordering columns and use those in assertions but it would require a more extensive refactor. I do think if there was an easy way to verify that the output data was correctly ordered (e.g. implementing a naive hand crafted sort that is inefficient but easy to verify for correctness) that would be nice, but it seems orthogonal to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2996942053
Thank you @AdamGS! It would be super helpful if we could first determine if the test is being overly sensitive to non-determinism or if the issue is actually reflecting incorrect query results or non-deterministic query results (e.g. the query is correct according to the sort order but the actual order of rows is different across runs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2994222512
We have the test in https://github.com/apache/datafusion/pull/16465/files#diff-f38cac7a9ac55c93d71632c96d6d2afa219cfb07351125a349099c86df859446 which seems to be passing. I'm running a local 1200 run iteration to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2996837290
> r. This causes some non-determinism with test runs since partitions can interact. I think this doesn't cause actual issues with queries, but the tests are picking it up.
This sounds like we need to update the tests to be deterministic then somehow (or ignore results that are not deterministic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3016874462
Sounds good, but how do we go about that? It's a pretty big change from "check results match row by row" to "check results are semantically correct".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3016847637
Amazing, you found it!
> Modify the fuzzer to account for the fact that this non-determinism is okay.
I think this is the way to go because the engine is correct in this case (sort doesn't care about partition/scan order, should just output top n rows in database).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3024615890
@Dandandan @alamb @AdamGS can you review and verify you agree with the proposed change to the tests? Would love to merge this and close this chapter in preparation for the next release 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-2994331327
So from my investigation what I *think* is happening is that https://github.com/apache/datafusion/pull/15770 fundamentally converted the TopK operation from being isolated per partition to having shared state via the dynamic filter. This causes some non-determinism with test runs since partitions can interact. I think this doesn't cause actual issues with queries, but the tests are picking it up. But I'm not 100% sure about that. @Dandandan and I were already talking about having a shared TopK heap between partitions, I think that would resolve the issue. But otherwise more investigation is needed.
FWIW the TopK dynamic filters still work without this code - it's just using the filter to filter rows in the TopK operator itself that doesn't work.
This is all I had time for today. I think more work is needed before we can merge this PR in the current state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3016782508
I've pushed a series of commits that simplify the test down to a trivial example:
| u64 | u32 |
|-----|-----|
| 1 | 1 |
| 2 | 1 |
```sql
SELECT * FROM t ORDER BY u32 LIMIT 1
```
Now it's clear what is happening: previously each partition had `ORDER BY u32 LIMIT 1` applied internally and then they got combined globally, so results were always deterministic.
With the addition of pre-filtering rows in the TopK operator, since the filter is updated across partitions, as soon as 1 partition sees the value for `u32` and has filled up it's limit the other partitions will drop the data on the floor. But which partition sees the value first and "wins" is non-deterministic, thus which value for `u64` comes out is also non deterministic. This isn't necessarily a bad thing, I believe many other database systems work like this (probably for similar reasons), but we do need to do something about this. Even if we had a shared TopK heap I think we'd have the same issue because which row ends up in the TopK heap will still be non-deterministic.
Options I can see:
1. Modify the fuzzer to account for the fact that this non-determinism is okay.
2. Make the filters be updated per-partition, which probably looses some performance (I think not too much?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore topk filtering tests [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3024235419
> Modify the fuzzer to account for the fact that this non-determinism is okay.
I agree this is the right way to go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] restore topk pre-filtering of batches and make sort query fuzzer less sensitive to expected non determinism [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #16501:
URL: https://github.com/apache/datafusion/pull/16501#issuecomment-3024724944
Since it seemed we came to a clear repro and conclusion and the best way to know if the fuzzer is still flakey or not is to run it a lot I went ahead and merged this so it gets shaken out a lot in CI before the next release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org