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

Replace generateIndexScan with costedIndexScan #2112

Closed

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Oct 29, 2023

Swap the index costing rules. This leaves the old code in, I will delete in a follow-up.

  • Full-text indexes were broken, we never used the indexes and there was a bug in FT index rebuilding when dropping/adding a PK. I fixed what I could and added index tests, we support indexes for the same subset of expressions that spatial are supported for.
  • I tried replicating most our hand-tuned index choosing logic. I added FDs so we can choose obviously best indexes when the option exists.
  • The costing will work when I add statistics and only fall back on manual rules when the cost of two indexes is identical.
  • This also patches up some cases that I missed before, like indexing InTuple.
  • Filter deleting is a lot better now, which is most of the plan changes.

sql/index.go Outdated Show resolved Hide resolved
@max-hoffman max-hoffman changed the base branch from max/costed-index-range-building to main November 2, 2023 19:03
@fulghum
Copy link
Contributor

fulghum commented Nov 2, 2023

  • Full-text indexes were broken, we never used the indexes and there was a bug in FT index rebuilding when dropping/adding a PK. I fixed what I could and added index tests, we support indexes for the same subset of expressions that spatial are supported for.

Great catch on the full-text indexes! That new CheckIndexedAccess field in ScriptTestAssertion is a really awesome addition. Not for this PR of course, but it would be cool to have the validation code that runs after enginetests check the integrity of fulltext indexes. I know they don't use a prolly tree for data storage, but still seems like a useful step to ensure integrity.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

CheckIndexedAccess is a great addition to our test framework. Very cool. It was interesting to see which fulltext actions are eligible to use the index! Not as many cases as I was expecting.

There were a few query plans that stopped using IndexedTableAccess; it looked liked they both used BETWEEN, so I'm guessing we just don't have support for turning that into an index range in the new code yet?

I'm still working through all the files, but wanted to go ahead and publish the current comments.

enginetest/queries/index_query_plans.go Outdated Show resolved Hide resolved
enginetest/queries/index_query_plans.go Outdated Show resolved Hide resolved
enginetest/queries/index_query_plans.go Outdated Show resolved Hide resolved
enginetest/evaluation.go Show resolved Hide resolved
@max-hoffman max-hoffman changed the base branch from main to max/costed-index-range-building November 6, 2023 16:05
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Adding a couple more comments on query plan changes before I have to run go meet someone.

While reviewing the diff in Goland, I also noticed on line 3817 in query_plans.go that we aren't using a range index for an in subquery. I don't see that diff in GitHub right now for some reason though... the query is: SELECT a.* FROM mytable a inner join mytable b on (a.i = b.s) WHERE a.s not in ('1', '2', '3', '4')

Comment on lines 624 to 625
//t.Skip("todo add tests and implement")

Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) is this todo still needed?

enginetest/queries/query_plans.go Show resolved Hide resolved
enginetest/queries/query_plans.go Show resolved Hide resolved
@max-hoffman
Copy link
Contributor Author

max-hoffman commented Nov 7, 2023

re line 3817, that's a good catch the plan is wrong (the same query plan with IN works, just not the NOT IN). Will fix shortly

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Finished digging through everything and didn't find anything else. Looks good!

Looks like the new Server Engine CI tests that Jennifer just added are failing for the TestFulltextIndexes tests though, because they can't find a matching full-text index.

@max-hoffman
Copy link
Contributor Author

Finished digging through everything and didn't find anything else. Looks good!

Looks like the new Server Engine CI tests that Jennifer just added are failing for the TestFulltextIndexes tests though, because they can't find a matching full-text index.

Yeah it looks like the context for those tests loses a reference to the index added in setup. Trying to debug the state lag right now. Thank you for the thorough review!

@Hydrocharged Hydrocharged deleted the max/switch-indexscan-rule branch February 7, 2024 13:45
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

Successfully merging this pull request may close these issues.

5 participants