Idx histogram manipulation #2142
Conversation
…o-mysql-server into max/idx-histogram-manipulation
There was a problem hiding this comment.
Looks good. The doc.go content was really nice – very helpful to readers to explain those major concepts.
I didn't dig into the tpcc plan changes, since I'm assuming they'll get reverted when you start ignoring partial statistic info.
Looks like you're still adding some tests and the safety check for partial stats; let me know if you want me to take another look at any of that. Everything here looked pretty solid.
sql/stats/statistic.go
Outdated
| @@ -130,6 +134,43 @@ func (s *Statistic) Histogram() sql.Histogram { | |||
| return buckets | |||
| } | |||
|
|
|||
| func (s *Statistic) WithDistinct(i uint64) sql.Statistic { | |||
There was a problem hiding this comment.
(super minor/nit) WithDistinctCount would be a little more consistent with other method names (e.g. WithRowCount, WithNullCount).
enginetest/queries/script_queries.go
Outdated
| @@ -98,6 +98,7 @@ type ScriptTestAssertion struct { | |||
|
|
|||
| // CheckIndexedAccess indicates whether we should verify the query plan uses an index | |||
| CheckIndexedAccess bool | |||
| IndexName string | |||
There was a problem hiding this comment.
I didn't dig into the details, but noticed we have an ExpectedIndexes []string field already. Seems like they're both trying to model the same thing. Any chance we can condense to just one?
| @@ -0,0 +1,942 @@ | |||
| package stats | |||
There was a problem hiding this comment.
| 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 |
| statistic: x2Stat, | ||
| pref: []interface{}{5}, | ||
| expBuckets: 2, | ||
| expRowCount: uint64(10), |
There was a problem hiding this comment.
Mostly just checking my understanding of the code... the BoundCnt param in Bucket indicates how many rows fall on the boundary (5 here), so would it be fair to say the expected row count is really 7 here instead of 10? (the 2 that fall on the boundary in the first bucket + the 5 in the next bucket)
Think that's something we can do to more accurately predict the cardinality when the prefix is equal to the boundary, or does that not work with other cases? (Or am I just misunderstanding the BoundCnt field? 😉)
There was a problem hiding this comment.
Yeah this is one of the optimizations that I missed a bit in this first version. Boundaries and most-common-value lists are keys+counts that should be checked to modify the distinct and count estimates more directly.
Add simple histogram mutators for filter types. Use histogram costs for index selection when available. Added
statsdocs.Dolt enginetests seem to be passing. Companion here: dolthub/dolt#6997
TODO: