opt: Add TryDecorrelateGroupBy rule #27342
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, 6 of 6 files at r2, 4 of 4 files at r3, 6 of 6 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/sql/opt/norm/decorrelate.go, line 219 at r3 (raw file):
// that is not-null. EnsureNotNullIfCountRows returns the input group, possibly // wrapped in a new Project if a new column was synthesized. func (c *CustomFuncs) EnsureNotNullIfCountRows(in, aggs memo.GroupID) memo.GroupID {
Can you add a bit of context about why this function is necessary? (Maybe just a reference to the rule that uses it)
pkg/sql/opt/norm/decorrelate.go, line 272 at r3 (raw file):
// appends an AnyNotNull aggregate function to the end of the given Aggregations // operator, and returns a new Aggregations operator. func (c *CustomFuncs) AppendNonKeyCols(in, aggs memo.GroupID) memo.GroupID {
Can you add some context about why this is needed? (Or a reference to the rule that uses it)
pkg/sql/opt/norm/decorrelate.go, line 325 at r4 (raw file):
} return c.f.InternGroupByDef(&memo.GroupByDef{ GroupingCols: groupingCols.Union(keyCols),
Add a comment that this function is only called on unordered GroupBy expressions.
pkg/sql/opt/optgen/cmd/optgen/rule_gen.go, line 245 at r1 (raw file):
isFirst := false isLast := false for i, item := range match.Items {
A comment would help here -- maybe just a reminder that ListAnyOp is ....
pkg/sql/opt/optgen/lang/doc.go, line 483 at r1 (raw file):
names = name ('|' name)* arg = bind and | ref | and and = and-input ('&' and)
what is and-input?
pkg/sql/opt/optgen/lang/doc.go, line 157 at r2 (raw file):
Child patterns within match and replace patterns can be "bound" to a named variable. These variables can then be referenced later in the match pattern or in the replace pattern. This is a critical part of the Optgen language, since
[nit] extra space
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/decorrelate.opt, line 240 at r3 (raw file):
=> (Select (GroupBy
Much nicer to be able to read here what the rule does!
pkg/sql/opt/optgen/cmd/optgen/rule_gen.go, line 269 at r1 (raw file):
} if isFirst {
[nit] it's hard to keep track of the if/elses here, it would be easier to read if it was something like
switch {
case isFirst && isLast: ...
case isFirst && !isLast: ...
case !isFirst && isLast: ...
case !isFirst && !isLast: ...
}
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/decorrelate.go, line 219 at r3 (raw file):
Previously, rytaft wrote…
Can you add a bit of context about why this function is necessary? (Maybe just a reference to the rule that uses it)
Done.
pkg/sql/opt/norm/decorrelate.go, line 272 at r3 (raw file):
Previously, rytaft wrote…
Can you add some context about why this is needed? (Or a reference to the rule that uses it)
Done.
pkg/sql/opt/norm/decorrelate.go, line 325 at r4 (raw file):
Previously, rytaft wrote…
Add a comment that this function is only called on unordered GroupBy expressions.
Done.
pkg/sql/opt/optgen/cmd/optgen/rule_gen.go, line 245 at r1 (raw file):
Previously, rytaft wrote…
A comment would help here -- maybe just a reminder that
ListAnyOpis....
Done.
pkg/sql/opt/optgen/cmd/optgen/rule_gen.go, line 269 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] it's hard to keep track of the
if/elses here, it would be easier to read if it was something likeswitch { case isFirst && isLast: ... case isFirst && !isLast: ... case !isFirst && isLast: ... case !isFirst && !isLast: ... }
Done.
pkg/sql/opt/optgen/lang/doc.go, line 483 at r1 (raw file):
Previously, rytaft wrote…
what is
and-input?
Should be expr. Fixed.
pkg/sql/opt/optgen/lang/doc.go, line 157 at r2 (raw file):
Previously, rytaft wrote…
[nit] extra space
Done.
There was a problem hiding this comment.
opt changes look good - what happened with all those libroach files?
Reviewed 8 of 12 files at r6, 6 of 6 files at r7, 2 of 4 files at r8, 6 of 6 files at r9.
Reviewable status:
pkg/sql/opt/norm/decorrelate.go, line 325 at r4 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
Done.
Doesn't look like that change was pushed.
Use same syntax for both match and replace patterns, in preparation for supporting binding in replace patterns. Release note: None
Previously, variable bindings were only allowed in match patterns. This
commit extends that to allow bindings in replace patterns as well. For
example:
(InnerJoin $left:* $right:* $on:*)
=>
(InnerJoin
$varname:(SomeFunc $left)
$varname2:(Select $varname (SomeOtherFunc $right))
(MakeOn $varname $varname2)
)
This is useful when an expression constructed in one part of a replace
pattern is shared or derived from another part.
Release note: None
Now that it's possible to bind expressions in the replace pattern, use that new functionality to improve the TryDecorrelateScalarGroupBy rule. Previously, construction of the replace pattern had to be done in a custom function. Now it can be done in the rule itself, using several helper custom functions. These same helper functions will also be used for a new rule that has a lot of overlap (TryDecorrelateGroupBy). Release note: None
This commit adds support for decorrelating non-scalar GroupBy expression,
such as this one:
SELECT *
FROM a
WHERE EXISTS
(
SELECT *
FROM xy
INNER JOIN (SELECT count(*) AS cnt FROM uv WHERE i=5 GROUP BY v) ON x=cnt
)
Decorrelation is similar to TryDecorrelateScalarGroupBy, and reuses several of
the helper functions created for it. This brings us one step closer to
decorrelating all the TPCH queries. Only some additional null-rejection work
remains.
Release note: None
There was a problem hiding this comment.
Hm, not sure. Fixed.
Reviewable status:
pkg/sql/opt/norm/decorrelate.go, line 325 at r4 (raw file):
Previously, rytaft wrote…
Doesn't look like that change was pushed.
Whoops, got removed when rebasing. I added the phrase "constructs a new unordered GroupByDef" in header comment, both for this and GroupByKey.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r11, 2 of 4 files at r12, 6 of 6 files at r13.
Reviewable status:
|
bors r+ |
27103: telemetry: add feature-usage counters and registry r=dt a=dt This is a prototype feature usage counter and registry, to allow easy any given feature or code path to easily record that it was hit without additional work required in the rest of the telemetry pipeline. The registry is global for now, to avoid significant plumbing and mess until we validate this is useful. Singletons often complicate testing but this registry's contents should not affect behavior, so some of those drawbacks are less applicable than in other sitations. If this pattern does prove useful, future work may need to find a more compact means of storing and transmitting bundled counts than their verbose string names -- perhaps moving the most common keys to fields of a pre-defined proto. Additionally, a lock-free map may provide superior performance compared to the lock-protected map. Release note: none. 27342: opt: Add TryDecorrelateGroupBy rule r=andy-kimball a=andy-kimball This series of commits adds a new TryDecorrelateGroupBy rule. It overlaps quite a bit with the existing TryDecorrelateScalarGroupBy rule. Therefore, this PR contains changes to Optgen that enable the two rules to be expressed more naturally in the Optgen language and to more easily share custom functions between themselves. Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Build succeeded |
This series of commits adds a new TryDecorrelateGroupBy rule. It
overlaps quite a bit with the existing TryDecorrelateScalarGroupBy rule.
Therefore, this PR contains changes to Optgen that enable the two
rules to be expressed more naturally in the Optgen language and to
more easily share custom functions between themselves.