-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
CostedIndexScan outline, non-functional #2093
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic makes sense. Just a couple of small comments/questions.
@@ -0,0 +1,63 @@ | |||
package stats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package stats | |
// Copyright 2023 Dolthub, Inc. | |
// | |
// Licensed under the Apache License, Version 2.0 (the "License"); | |
// you may not use this file except in compliance with the License. | |
// You may obtain a copy of the License at | |
// | |
// http://www.apache.org/licenses/LICENSE-2.0 | |
// | |
// Unless required by applicable law or agreed to in writing, software | |
// distributed under the License is distributed on an "AS IS" BASIS, | |
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
// See the License for the specific language governing permissions and | |
// limitations under the License. | |
package stats |
} | ||
} | ||
|
||
iat := rt.Table.(sql.IndexAddressableTable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor) is this type cast worth checking? Seems like some edge cases (some system tables?) may not implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this any non-ResolvedTable sql.TableNode
types ended up being bugs
(4: and | ||
(3: xy.x = 1)) | ||
(5: and | ||
(4: xy.y = 2)) | ||
(5: and | ||
(6: xy.y > 0) | ||
(7: xy.y < 7)))`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we create the parent and
expressions on line 85 and 87 over the or
s? In particular, it seemed odd that 5: and
contained expression child number 4
when it doesn't seem to in the original expression.
I was expecting a flattened form more like:
(1: or
(3: xy.x = 1))
(4: xy.y = 2))
(5: and
(6: xy.y > 0)
(7: xy.y < 7)))`,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I did this is because it's easy to say an OR's children are a list of ANDs. If that's confusing I can make the tree look more similar to the original version, the code becomes a little more complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numbering being weird might be another good reason to do it the way you suggest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the number overlaps were getting in the way of filter pruning, index_query_plans.go is now -11k
de5d0f1
to
98924b6
Compare
This is an outline for proposed new costed index scan. I stopped here because I got most of the shaped flushed out enough to get the first component (flatten expression tree) tested and functional.
This is also explained in the docstring but the process is:
Filter->ResolvedTable
patterns. Filter pushdown is good enough so that all filters usable in range scans should now sit on top of their target tables.sql.RangeCollection->sql.IndexLookup->plan.IndexedTableAccess
. We walk the expression root the same way as we did in the other steps, using a bitmask to determine which filters should be converted to ranges, and using the existing ansql.IndexBuilder
to convert leaf expressions into ranges (plus manually INTERSECT/UNIONing the children of AND/OR filters).TODO:
stats
helper functions that emulate leaf filter behavior (truncate, intersect, union statistics)sql.RangeCollection
Other:
analyzer
.