Skip to content

Commit

Permalink
Merge #129050
Browse files Browse the repository at this point in the history 
129050: opt: disable `GenerateParameterizedJoin` when forcing custom plans r=mgartner a=mgartner

#### opt: remove documentation of old flags

In #85935 we introduced the `set` flag to set session settings in
optimizer tests and a few flags that were already controlling session
settings were removed. The documentation of those old flags has been
removed.

Release note: None

#### opt: disable `GenerateParameterizedJoin` when forcing custom plans

The exploration rule `GenerateParameterizedJoin` is now disabled when
the `plan_cache_mode` session setting is set to `force_custom_plan`.
This prevents possible regressions in plans for this mode in the rare
case that a stable function is not folded during normalization (due to
an error encountered during folding). In most cases, the check for
placeholders and stable functions in `GenerateParameterizedJoin` is
sufficient for preventing the rule from firing when a generic query plan
is not being built—before optimizing a custom plan placeholders are
always replaced with constants and stable functions are usually folded
to constants.

Epic: None

Release note: None

#### sql: fix minor typo in plan_opt.go

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
@mgartner
craig[bot] and mgartner committed Aug 22, 2024
2 parents 8d2bd09 + 2d0ea24 commit 1decd78
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 48 deletions.
1 change: 1 addition & 0 deletions pkg/sql/opt/norm/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ go_test(
"//pkg/sql/sem/catconstants",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/sessiondatapb",
"//pkg/sql/types",
"//pkg/testutils/datapathutils",
"//pkg/util/leaktest",
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/opt/norm/factory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/types"
)

Expand Down Expand Up @@ -84,6 +85,7 @@ func TestCopyAndReplace(t *testing.T) {
}

evalCtx := eval.MakeTestingEvalContext(cluster.MakeTestingClusterSettings())
evalCtx.SessionData().PlanCacheMode = sessiondatapb.PlanCacheModeAuto

var o xform.Optimizer
testutils.BuildQuery(t, &o, cat, &evalCtx, "SELECT * FROM ab WHERE a = $1")
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/testutils/opttester/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ go_library(
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/sem/volatility",
"//pkg/sql/sessiondatapb",
"//pkg/sql/stats",
"//pkg/testutils/datapathutils",
"//pkg/testutils/floatcmp",
Expand Down
28 changes: 12 additions & 16 deletions pkg/sql/opt/testutils/opttester/opt_tester.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/stats"
"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
"github.com/cockroachdb/cockroach/pkg/testutils/floatcmp"
Expand Down Expand Up @@ -168,6 +169,9 @@ type Flags struct {
// memo/check_expr.go.
DisableCheckExpr bool

// Generic enables optimizations for generic query plans.
Generic bool

// ExploreTraceRule restricts the ExploreTrace output to only show the effects
// of a specific rule.
ExploreTraceRule opt.RuleName
Expand Down Expand Up @@ -477,6 +481,11 @@ func New(catalog cat.Catalog, sqlStr string) *OptTester {
//
// - disable-check-expr: skips the assertions in memo/check_expr.go.
//
// - generic: enables optimizations for generic query plans.
// NOTE: This flag sets the plan_cache_mode session setting to "auto", which
// cannot be done via the "set" flag because it requires a CCL license,
// which optimizer tests are not set up to utilize.
//
// - rule: used with exploretrace; the value is the name of a rule. When
// specified, the exploretrace output is filtered to only show expression
// changes due to that specific rule.
Expand All @@ -502,12 +511,6 @@ func New(catalog cat.Catalog, sqlStr string) *OptTester {
// - table: used to set the current table used by the command. This is used by
// the inject-stats command.
//
// - stats-quality-prefix: must be used with the stats-quality command. If
// rewriteActualFlag=true, indicates that a table should be created with the
// given prefix for the output of each subexpression in the query. Otherwise,
// outputs the name of the table that would be created for each
// subexpression.
//
// - ignore-tables: specifies the set of stats tables for which stats quality
// comparisons should not be outputted. Only used with the stats-quality
// command. Note that tables can always be added to the `ignore-tables` set
Expand All @@ -523,16 +526,6 @@ func New(catalog cat.Catalog, sqlStr string) *OptTester {
//
// - inject-stats: the file path is relative to the test file.
//
// - join-limit: sets the value for SessionData.ReorderJoinsLimit, which
// indicates the number of joins at which the optimizer should stop
// attempting to reorder.
//
// - prefer-lookup-joins-for-fks: sets SessionData.PreferLookupJoinsForFKs to
// true, causing foreign key operations to prefer lookup joins.
//
// - null-ordered-last: sets SessionData.NullOrderedLast to true, which orders
// NULL values last in ascending order.
//
// - cascade-levels: used to limit the depth of recursive cascades for
// build-cascades.
//
Expand Down Expand Up @@ -1003,6 +996,9 @@ func (f *Flags) Set(arg datadriven.CmdArg) error {
case "disable-check-expr":
f.DisableCheckExpr = true

case "generic":
f.evalCtx.SessionData().PlanCacheMode = sessiondatapb.PlanCacheModeAuto

case "rule":
if len(arg.Vals) != 1 {
return fmt.Errorf("rule requires one argument")
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/opt/xform/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ go_library(
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/sem/volatility",
"//pkg/sql/sessiondatapb",
"//pkg/sql/types",
"//pkg/util/buildutil",
"//pkg/util/cancelchecker",
Expand Down
7 changes: 7 additions & 0 deletions pkg/sql/opt/xform/generic_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,16 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/types"
)

// GenericRulesEnabled returns true if rules for optimizing generic query plans
// are enabled, based on the plan_cache_mode session setting.
func (c *CustomFuncs) GenericRulesEnabled() bool {
return c.e.evalCtx.SessionData().PlanCacheMode != sessiondatapb.PlanCacheModeForceCustom
}

// HasPlaceholdersOrStableExprs returns true if the given relational expression's subtree has
// at least one placeholder.
func (c *CustomFuncs) HasPlaceholdersOrStableExprs(e memo.RelExpr) bool {
Expand Down
4 changes: 3 additions & 1 deletion pkg/sql/opt/xform/rules/generic.opt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
#
[GenerateParameterizedJoin, Explore]
(Select
$scan:(Scan $scanPrivate:*) & (IsCanonicalScan $scanPrivate)
$scan:(Scan $scanPrivate:*) &
(GenericRulesEnabled) &
(IsCanonicalScan $scanPrivate)
$filters:* &
(HasPlaceholdersOrStableExprs (Root)) &
(Let
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/opt/xform/testdata/external/hibernate
View file
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,7 @@ project
└── filters
└── min:14 = $1 [outer=(14), constraints=(/14: (/NULL - ]), fd=()-->(14)]

opt
opt generic
select
person0_.id as id1_2_,
person0_.address as address2_2_,
Expand Down Expand Up @@ -951,7 +951,7 @@ project
└── filters
└── max:16 = 0 [outer=(16), constraints=(/16: [/0 - /0]; tight), fd=()-->(16)]

opt
opt generic
select
person0_.id as id1_2_,
person0_.address as address2_2_,
Expand Down Expand Up @@ -1016,7 +1016,7 @@ project
│ └── filters (true)
└── filters (true)

opt
opt generic
select
person0_.id as id1_2_,
person0_.address as address2_2_,
Expand Down
16 changes: 8 additions & 8 deletions pkg/sql/opt/xform/testdata/external/nova
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ create table instance_type_extra_specs
)
----

opt
opt generic
select anon_1.flavors_created_at as anon_1_flavors_created_at,
anon_1.flavors_updated_at as anon_1_flavors_updated_at,
anon_1.flavors_id as anon_1_flavors_id,
Expand Down Expand Up @@ -710,7 +710,7 @@ sort
└── filters
└── instance_type_extra_specs_1.instance_type_id:22 = instance_types.id:1 [outer=(1,22), constraints=(/1: (/NULL - ]; /22: (/NULL - ]), fd=(1)==(22), (22)==(1)]

opt
opt generic
select anon_1.instance_types_created_at as anon_1_instance_types_created_at,
anon_1.instance_types_updated_at as anon_1_instance_types_updated_at,
anon_1.instance_types_deleted_at as anon_1_instance_types_deleted_at,
Expand Down Expand Up @@ -861,7 +861,7 @@ project
│ └── instance_type_extra_specs_1.deleted:37 = $7 [outer=(37), constraints=(/37: (/NULL - ]), fd=()-->(37)]
└── filters (true)

opt
opt generic
select anon_1.instance_types_created_at as anon_1_instance_types_created_at,
anon_1.instance_types_updated_at as anon_1_instance_types_updated_at,
anon_1.instance_types_deleted_at as anon_1_instance_types_deleted_at,
Expand Down Expand Up @@ -1026,7 +1026,7 @@ project
│ └── instance_type_extra_specs_1.deleted:37 = $7 [outer=(37), constraints=(/37: (/NULL - ]), fd=()-->(37)]
└── filters (true)

opt
opt generic
select anon_1.flavors_created_at as anon_1_flavors_created_at,
anon_1.flavors_updated_at as anon_1_flavors_updated_at,
anon_1.flavors_id as anon_1_flavors_id,
Expand Down Expand Up @@ -1166,7 +1166,7 @@ project
│ └── filters (true)
└── filters (true)

opt
opt generic
select anon_1.flavors_created_at as anon_1_flavors_created_at,
anon_1.flavors_updated_at as anon_1_flavors_updated_at,
anon_1.flavors_id as anon_1_flavors_id,
Expand Down Expand Up @@ -1609,7 +1609,7 @@ sort
└── filters
└── instance_type_extra_specs_1.instance_type_id:36 = instance_types.id:1 [outer=(1,36), constraints=(/1: (/NULL - ]; /36: (/NULL - ]), fd=(1)==(36), (36)==(1)]

opt
opt generic
select anon_1.instance_types_created_at as anon_1_instance_types_created_at,
anon_1.instance_types_updated_at as anon_1_instance_types_updated_at,
anon_1.instance_types_deleted_at as anon_1_instance_types_deleted_at,
Expand Down Expand Up @@ -2357,7 +2357,7 @@ sort
└── filters
└── instance_type_extra_specs_1.instance_type_id:50 = instance_types.id:1 [outer=(1,50), constraints=(/1: (/NULL - ]; /50: (/NULL - ]), fd=(1)==(50), (50)==(1)]

opt
opt generic
select anon_1.instance_types_created_at as anon_1_instance_types_created_at,
anon_1.instance_types_updated_at as anon_1_instance_types_updated_at,
anon_1.instance_types_deleted_at as anon_1_instance_types_deleted_at,
Expand Down Expand Up @@ -2511,7 +2511,7 @@ project
│ └── instance_type_extra_specs_1.deleted:37 = $7 [outer=(37), constraints=(/37: (/NULL - ]), fd=()-->(37)]
└── filters (true)

opt
opt generic
select anon_1.flavors_created_at as anon_1_flavors_created_at,
anon_1.flavors_updated_at as anon_1_flavors_updated_at,
anon_1.flavors_id as anon_1_flavors_id,
Expand Down
Loading

0 comments on commit 1decd78

Please sign in to comment.