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.

Sign up for GitHub

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

Jump to bottom

Start on better indexing support #2223

Merged
merged 12 commits into from
Mar 25, 2025
Merged

Start on better indexing support #2223

merged 12 commits into from
Mar 25, 2025

Conversation

josephschorr
Copy link
Member

@josephschorr josephschorr commented Jan 31, 2025 •
edited by vroldanbet
Loading

  • Adds specialized structs for defining indexes as part of the DB schema
  • Adds explain support for SQL-based datastore drivers
  • Adds a DS proxy that enforces that expected indexes are used for marked query shapes
  • Adds first set of query shapes
  • Refactored some migrations for the sake of exercising the new common.IndexDefinition API

Follow ups

@github-actions github-actions bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Jan 31, 2025
@kartikaysaxena
Copy link
Contributor

great add!

@kartikaysaxena kartikaysaxena mentioned this pull request Jan 31, 2025
Closed
@josephschorr josephschorr force-pushed the better-indexing branch 2 times, most recently from 37e52d4 to 2e8682a Compare March 17, 2025 15:52
@josephschorr josephschorr marked this pull request as ready for review March 17, 2025 18:26
@josephschorr josephschorr requested review from vroldanbet and a team as code owners March 17, 2025 18:26
vroldanbet
vroldanbet reviewed Mar 18, 2025
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first batch of comments

internal/datastore/mysql/version/version.go
@@ -3,4 +3,4 @@ package version
// MinimumSupportedMySQLVersion is the minimum version of MySQL supported for this driver.
//
// NOTE: must match a tag on DockerHub for the `mysql` image.
const MinimumSupportedMySQLVersion = "8.0"
const MinimumSupportedMySQLVersion = "8.4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this bumped?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its the minimum supported version by MySQL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this warrants at least a note in the release, even if those versions are already EoL

vroldanbet
vroldanbet reviewed Mar 18, 2025
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last batch of review comments

internal/datastore/common/relationships.go
}

err = tx.QueryFunc(ctx, func(ctx context.Context, rows R) error {
explainString := ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a StringBuilder since this could be a pretty long explain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only done during testing; we don't really care about performance in that case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's death by a thousand cuts. Then we start asking ourselves "why is CI taking so long?". I think keeping tests as fast as the critical path is also important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree; sometimes you don't want to hyper optimize code (making it less readable) simply to ensure some small improvements on CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using a StringBuilder to replace string concatenation falls under "hyper optimize" category, we know it can have quite an impact in hot paths, but sure

@josephschorr
Copy link
Member Author

Updated

@josephschorr josephschorr added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2025
@josephschorr josephschorr added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2025
@josephschorr josephschorr added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2025
@josephschorr josephschorr added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2025
@josephschorr josephschorr added this pull request to the merge queue Mar 25, 2025
Merged via the queue into authzed:main with commit a017482 Mar 25, 2025
41 checks passed
@josephschorr josephschorr deleted the better-indexing branch March 25, 2025 12:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vroldanbet vroldanbet vroldanbet approved these changes

Assignees
No one assigned
Labels
area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@josephschorr @kartikaysaxena @vroldanbet