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

change modeling of OrderingPart to use a sort order instead of a boolean for reversedness #2745

Open
normen662 opened this issue May 29, 2024 · 1 comment

Comments

@normen662
Copy link
Contributor

This is related to #1318. It would be useful to state that certain ordering parts are fixed, i.e. bound to one value vie an e.g. equality predicate. In those cases forward and backward become meaningless, they would both fit.

This issue addresses replacing the isReverse with a new enum SortOrder which can be ASCENDING, DESCENDING, and FIXED.

There are some places in the codebase where we reason about Ordering and in particular OrderingParts where we rely on the equality of OrderingParts. An ordering part (q.a, ASC) is equal to (q.a, ASC) only, in particular it is not equal to (q.a, DESC), (q.a, FIXED).

Unfortunately, a RequestedOrdering which is used to enumerate satisfying orderings does not want equality but just compatibility, i.e. a requested ordering of (q.a, ASC) or (q.a, DESC) are both compatible with (q.a, FIXED) even though they are not equal. All of this is part of Ordering.enumerateSatisfyingOrderings() and Ordering.enumerateSatisfyingOrderingComparisonKeyValues().

Similarly, merging two Orderings has a similar problem. An ordering (q.a, ASC) and an ordering (q.a, FIXED) should be merged to (q.a, ASC). The code that needs to be adapted is in Ordering.mergePartialOrderOfOrderings() which (again) only considers equality instead of compatibility.

@alecgrieser
Copy link
Contributor

As a note, having a separate FIXED ordering may help with #2723, though that issue is exacerbated by some other problems with how values are updated during pullUp that we probably want to fix, so maybe we deal with that separately.

MMcM pushed a commit that referenced this issue Jul 1, 2024
* add sort order to ordering parts

* savepoint

* Ordering class completed

* everything refactored except OrderingProperty

* OrderingTest works again

* more fixes

* pullup constants

* new in-union plans break a bunch of set op creation

* save point

* savepoint 2

* fixing tests

* tests nd checkstyle pass

* changes to the basic data access rules to resolve reverse order properly

* tests pass

* code complete; all tests pass

* writing documentation

* savepoint

* all tests pass

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

No branches or pull requests

2 participants