v3: fix a panic and refactor for future primitives #4854
Conversation
The previous implementation separated out primitives that could handle grouping from those who couldn't. This new approach allows each primitive to make its own decision about whethere it can handle grouping or not. We still continue to perform the high level checks before pushing group by functions. This new approach will work better as our ability to handle more constructs evolve, because every primitive will have to find a way to make grouping work. Additionally, the order of primitives is now being maintained more accurately. This makes the code more future-proof. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Just like the work done for grouping. All primitives have been changed to handle ORDER BY. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Fixes vitessio#4851 A route that performs a merge-sort should not be treated as a traditional route because many constructs cannot be pushed down into it. This change creates a pseudo-primitive to prevent this confusion. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
go/vt/vtgate/planbuilder/subquery.go
Outdated
| func (sq *subquery) PushOrderByRand() { | ||
| // PushGroupBy satisfies the builder interface. | ||
| func (sq *subquery) PushGroupBy(_ sqlparser.GroupBy) error { | ||
| return errors.New("unupported: group by on cross-shard subquery") |
There was a problem hiding this comment.
| return errors.New("unupported: group by on cross-shard subquery") | |
| return errors.New("unsupported: group by on cross-shard subquery") |
There was a problem hiding this comment.
Why didn't this fail a unit test?
There was a problem hiding this comment.
This code is unreachable because there are higher level checks that catch this case. But it can get called in the future as things evolve.
It's something one has to keep in mind while making changes: review code coverage of tests and make sure reachable code is reached.
| @@ -329,30 +329,5 @@ | |||
| } | |||
| } | |||
|
|
|||
| # limit and order by null clauses | |||
There was a problem hiding this comment.
my understanding is that we still support order by null, so why has this testcase been deleted?
There was a problem hiding this comment.
It previously ignored order by null (and rand), which was kind of incorrect, especially for rand. So, I decided to instead go with the simplified rule that you shouldn't order by anything for vindex functions.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Previously group by and order by constructs were treated as special case. The new change extends the builder interface and requires all primitives to implement the corresponding push functions.
This approach allows for primitives to be composed more easily because of the fact that we don't special case these constructs any more.
This also fixes #4851. The above changes make it easy to introduce a new mergeSort pseudo primitive that differentiates regular routes from those that perform a merge-sort. This prevents push-down of some constructs that were previously (incorrectly) allowed.