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

Collations Integration Ordering #9155

Merged
merged 8 commits into from
Nov 15, 2021
Merged

Collations Integration Ordering #9155

merged 8 commits into from
Nov 15, 2021

Conversation

king-11
Copy link
Contributor

@king-11 king-11 commented Nov 6, 2021

Description

Engine Update

This PR adds collation support to the engine module specifically focused on making comparer struct collation aware by including collation ID in it which can then be passed down to the NullSafeCompare responsible for actually comparing values.

type comparer struct {

To be able to pass down collation ID into the comparer struct we need to contain it further up the module level i.e. in OrderByParams.

type OrderByParams struct {

Plan Builder Updates

  • As the params are populated in planBuilder it becomes trivial to extract collation ID in most cases.

Memory Sort on Aggregation

The plan builder tests were updated so that testing can be done for createMemorySortPlanOnAggregation. For that, the OrderByParams.String() was updated to include the collation name with COLLATE keyword. This changes the description of route for which tests were updated this also allows us to test for createMemorySortPlanOnAggregation using the plan builder tests.

v3

We don't have the semantic table for the v3 planner code and we intend to only add collations support to Gen4 only, after discussing with @systay @frouioui. Hence the TODOs are left in ordering.go and memory_sort.go.

Tests Added

  • Route Sort with Collation
  • Memory Sort with Collation
  • Merge Sort with Collation

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@king-11 king-11 marked this pull request as ready for review November 10, 2021 07:57
@vmg
Copy link
Collaborator

vmg commented Nov 10, 2021

The test failures in the unit tests are not flukes. Looks like you need to tweak the code -- a good starting point would be the slice access that is causing a panic.

@king-11 king-11 force-pushed the compare branch 2 times, most recently from 48b820f to 5d58b0c Compare November 11, 2021 03:47
@king-11
Copy link
Contributor Author

king-11 commented Nov 11, 2021

@vmg the issue is resolved i was trying to access KeyCol instead of actual index

@vmg vmg added Component: Query Serving release notes none Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Nov 11, 2021
@king-11 king-11 force-pushed the compare branch 2 times, most recently from 27bf673 to dd57d71 Compare November 12, 2021 08:34
Signed-off-by: Lakshya Singh <[email protected]>
cases for asc, desc, error and unsupported
Signed-off-by: Lakshya Singh <[email protected]>
no way to get collation from aggregates use group by only with if check
Signed-off-by: Lakshya Singh <[email protected]>
Signed-off-by: Lakshya Singh <[email protected]>
testing order by with aggregation using plan tests

Co-authored-by: Florent Poinsard <[email protected]>
Co-authored-by: Lakshya Singh <[email protected]>

Signed-off-by: Lakshya Singh <[email protected]>
Signed-off-by: Lakshya Singh <[email protected]>
@vmg vmg merged commit 0f6b9ed into vitessio:main Nov 15, 2021
@king-11 king-11 deleted the compare branch November 15, 2021 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants