-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Stack overflow calling EqvalenceProperties::project
with certain order
#12700
Comments
Here is the stack trace:
|
@mustafasrepo @ozankabak or @berkaysynnada does this code look familiar to you? Any hints on where to begin? I looked into it briefly this morning and it appears the dependencies computed are cyclic |
Hmm, will take a look. cc @akurmustafa |
You are right - construct_orderings() seems to be the source of the problem. Unfortunately, I am not able to dive into it today. |
If no one beats us to it, we will get to this after finishing some firefighting |
Thanks -- I may also find time to work on it, but will post here before doing so |
I couldn't look this in detail. However, posting my thoughts, in case that helps. Given orderings are Similary, for the orderings |
I am starting to check this out |
I am still trying to convince myself that this is the correct transformation for the most condensed ordering 🤔 I will keep thinking about it. I poked around trying to find a function that could collapse the ordering I will also try and make a PR to add some comments about the "no redundant information" invariant |
Does this simplification make sense?
|
Here is a PR that fixes the issue #12759 -- however, given the discussions above there may well be a deeper issue we need to solve too |
I will investigate further into the case where:
as @akurmustafa mentioned, I have some questions about what the final state of ordering equivalence is and what it should be, FYI |
Thank you |
I have worked through this comment in detail to determine whether there is an actual bug in the current implementation. Initially, my thoughts were similar to @akurmustafa's intuition, and I draft a few designs. However, I can now safely say that the current implementation is correct, and the tests are valid for both the The example that triggered the initial bug was Why? Consider this table:
This table is ordered by both The answer is no. Consider this table:
This table also satisfies both If someone needs to infer an ordering corresponding to the first table, it should be given as In short:
|
thank you for reviewing and double checking @berkaysynnada -- very much appreciated |
Thanks @berkaysynnada for this finding. Nice catch. I hope, I didn't waste your time by misdirecting. |
Describe the bug
Given equivalence sort orders like this:
When
EqvalenceProperties::project
is called to projecta, b, c, d
the stack overflowsTo Reproduce
Reproducer:
Full Diff
Then run
Expected behavior
Test should pass (expected output will need to be updated)
Additional context
I found this while working on #12446 / #12562
The text was updated successfully, but these errors were encountered: