refactor: Rework limit node flow #767
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewSisley
added
area/query
shahzadlone
reviewed
Aug 30, 2022
| return false | ||
| } | ||
|
|
||
| if !s.Limit.equal(other.Limit) { |
There was a problem hiding this comment.
todo: Since you are adding this new Limit comparison, please update the above Note.
suggestion(non-blocking): Removal of these lines:
// Note: Currently only compares Name and Filter as that is all that is currently required,
// but this should be extended in the future.
There was a problem hiding this comment.
Ah cheers, I saw that and forgot about it by the time I'd fixed the tests. Changing.
- Update comment
shahzadlone
approved these changes
Aug 30, 2022
Codecov Report
@@ Coverage Diff @@
## develop #767 +/- ##
===========================================
- Coverage 58.36% 58.31% -0.05%
===========================================
Files 147 147
Lines 16833 16780 -53
===========================================
- Hits 9824 9786 -38
+ Misses 6084 6072 -12
+ Partials 925 922 -3
|
Some loss of efficiency here, I think the new limit nodes will make use of the same valuesNode (so not fetching from file twice) but not sure. The planner should be able to optimise this better in the future by using the (bigger) limitNode as the source of the smaller limit node or something similar.
AndrewSisley
force-pushed
the
sisley/refactor/I764-limit-rework
branch
from
4a40320
to
585c202
Compare
|
I rebased the aggregate-limit branch onto this one and can confirm that it solves the problem |
fredcarle
approved these changes
Aug 30, 2022
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Add tests for one-many limits * Simplify limit logic to make use of mapper Some loss of efficiency here, I think the new limit nodes will make use of the same valuesNode (so not fetching from file twice) but not sure. The planner should be able to optimise this better in the future by using the (bigger) limitNode as the source of the smaller limit node or something similar.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #764
Description
Reworks the limit logic, removing the 2 limit node types and instead relying on mapper to to add a new backing field if the limits are not equal. Should make it much much easier to add (full) limit support for aggregates, as previously they were tripping up on the deleted lines in planner (RE parent aggregates), this task was broken out of that piece of work as I saw it as an easy way to break up the PR, and a valuable improvement to the code in its own right.
There is some loss of efficiency here, I think the new limit nodes will make use of the same valuesNode (so not fetching from file twice) but not sure. The planner should be able to optimize this better in the future by using the (bigger) limitNode as the source of the smaller limit node or something similar. I am happy saving that plan optimization for another day, but please let me know your thoughts on this.
Specify the platform(s) on which this was tested: