release-23.2: opt/memo: use virtual column stats in statistics builder #121179
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
blathers-crl
bot
added
the
backport
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
This commit reduces allocation in `statisticsBuilder.colStatJoin`. Previously, it was creating intersections of two sets, which, in some cases, were only useful for checking their emptiness. Now we use the `ColSet.Intersects` method which returns a boolean and does not build a new set. Release note: None
As of cockroachdb#118241 we now collect table statistics on virtual computed columns, but do not yet use them in statistics builder. The difficulty with using these stats in statistics builder is that virtual computed columns are synthesized by various non-Scan expressions (Project, Select, etc). When calculating stats for these non-Scan expressions, we need to find the virtual column stats even though the virtual columns are not produced by the input to these expressions. To solve this, we add a VirtualCols set to props.Statistics which holds all of the virtual columns that could be produced by the input to a group. Expressions that could synthesize virtual columns will look in this set to discover whether there are statistics for any of the scalar expressions they render. If there are, they will call colStatXXX using the virtual column ID as if the virtual column had originated from their input. This commit adds VirtualCols but does not yet use it. Note that we cannot currently pass VirtualCols up through set operations or with-scans, due to the column ID translation they use. Informs: cockroachdb#68254 Epic: CRDB-8949 Release note: None
Informs: cockroachdb#68254 Epic: CRDB-8949 Release note (sql): Add new session variable `optimizer_use_virtual_computed_column_stats`. When this variable is enabled, the optimizer will make use of table statistics on virtual computed columns.
Throughout statistics builder we use OutputCols to determine which columns come from the input to an expression. We then typically call colStatXXX with those columns as part of statistics calculation. In order to use statistics on virtual computed columns, we need to call colStatXXX on any virtual columns that could come from our input, even if they are not passed upward through OutputCols. To do this we extend OutputCols with the VirtualCols set we built in a previous commit. This commit replaces almost all usages of OutputCols in statistics builder with a call to helper function colStatCols, which returns a union of OutputCols and VirtualCols. This is enough to get the optimizer to use statistics on virtual computed columns in some simple plans. More complex plans will require matching the virtual column scalar expressions, which will be in the next PR. I've left some TODOs marking spots where this next PR will touch. Informs: cockroachdb#68254 Epic: CRDB-8949 Release note: None
As of cockroachdb#120668 we now use statistics on virtual computed columns in statistics builder. Simple queries that synthesize virtual columns in Project expressions already benefit, because they use the virtual column ID when synthesizing the virtual column. Other expressions, however, do not directly use the virtual column ID when synthesizing a virtual column. This includes Select expressions, joins, constrained scans, and some Project expressions. For example, consider a query like the following: ``` CREATE TABLE ab (a INT PRIMARY KEY, b INT AS (a % 10) VIRTUAL, INDEX (b)); SELECT * FROM ab WHERE a % 10 > 3; ``` Even though the filter condition is in terms of `a`, we'd like to use the statistics on virtual computed column `b` since the expression matches. In order to do this, we replace `a % 10` with `b` in a copy of the filter condition before doing any stats calculations. Then we perform our normal stats calculations, using `b`. Fixes: cockroachdb#68254 Fixes: cockroachdb#110146 Epic: CRDB-8949 Release note: None
With optimizer_use_virtual_computed_column_stats set to false, constrained scans were still sometimes using stats on virtual computed columns. This commit adds a check to makeTableStatistics which prevents creation of any statistics referencing a virtual computed column, which is a stronger check than existed before. With this check, the VirtualCols sets will always be empty when optimizer_use_virtual_computed_column_stats is false. Informs: cockroachdb#68254 Epic: CRDB-8949 Release note: None
michae2
force-pushed
the
backport23.2-114623-120668-120875-121171
branch
from
91239ec
to
a4f2275
Compare
All of the changes are behind session variable
Risks:
Statistics estimates for queries on indexed virtual computed columns will continue to be inaccurate, possibly prohibiting creation of indexes on virtual computed columns.
It cannot wait. |
|
The backport was fairly clean. Differences from master:
|
|
Was |
Yes, the fourth commit touches most of the same lines as this commit, so I included it to avoid merge skew. (I would have ended up re-creating it anyway in order to fix the fourth commit, so it was cleaner this way.) |
|
(Failures are known flake #120775 and what looks like an unrelated timeout under stressrace, btw.) |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 12 of 12 files at r2, 8 of 8 files at r3, 2 of 2 files at r4, 4 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2, @rafiss, and @rytaft)
pkg/sql/opt/memo/statistics_builder.go line 258 at r4 (raw file):
if sb.evalCtx.SessionData().OptimizerUseVirtualComputedColumnStats && !props.Statistics().VirtualCols.Empty() { return props.OutputCols.Union(props.Statistics().VirtualCols)
I can see now one of the reasons you had this new set contain both output and virtual columns originally (IIRC): it could be used directly here instead of creating a new set with Union and potentially allocating. No need to change back, but something we can maybe reconsider if this ever shows up in a profile.
pkg/sql/opt/memo/statistics_builder.go line 4977 at r5 (raw file):
expr opt.Expr } virtExprs := make([]virtExpr, virtualCols.Len())
nit: We could probably store this in statisticsBuilder and reuse the allocated slice everytime the function is called.
There was a problem hiding this comment.
Thank you!
Reviewable status:
pkg/sql/opt/memo/statistics_builder.go line 4977 at r5 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: We could probably store this in
statisticsBuilderand reuse the allocated slice everytime the function is called.
Drew had this feedback, too. It makes sense, but I'm a little wary, because I believe it's possible to re-enter this function during the replacement step. I haven't proven this, but during replacement we construct new expressions, and so it might be possible that we construct a new group which then has logical props and statistics built. While building stats we might call back into this function.
So just to play it safe, seems better to keep the slice local.
(Maybe I should leave this as a comment in the code?)
|
I'll go ahead and merge so we get some test coverage over the weekend. |
Backport:
Please see individual PRs for details.
/cc @cockroachdb/release
Release justification: high priority business need for the functionality which cannot wait until the next release.