sql: hoist project session flag and support session settings in opt tests

[msirek]

sql: This commit adds the disable_hoist_projection_in_join_limitation session flag.

Release note: none

opttester: This commit adds the set opttest flag which can be used to set
session flags via "set=flagname=value".

Release note: none

opttester: This commit removes session setting flags from OptTester.Flags

Release note: none

Release justification: Low risk fix for forwards compatibility of session flag which is
being backported.

[cockroach-teamcity]

This change is 

[mgartner]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

---

-- commits line 2 at r1:
nit: prefix the commit title with sql:

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 574 at r2 (raw file):

	ot.evalCtx.SessionData().PropagateInputOrdering = ot.Flags.PropagateInputOrdering
	ot.evalCtx.SessionData().NullOrderedLast = ot.Flags.NullOrderedLast
	ot.evalCtx.SessionData().OptimizerUseMultiColStats = ot.Flags.UseMultiColStats

The current pattern is to set the session variable here rather than introducing the SetSessionVariable function.

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

// true.
func (c *CustomFuncs) CanHoistProjectInput(relation memo.RelExpr) (ok bool) {
	if c.e.f.EvalContext() != nil &&

nit: I don't see the same nil checks in other places, are you sure it's necessary?

Also, you can access the eval context from c.e.evalCtx.

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

---

-- commits line 2 at r1:

Previously, mgartner (Marcus Gartner) wrote…

nit: prefix the commit title with sql:

By title, do you mean the PR title or this text line? I've updated the PR title.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 574 at r2 (raw file):

	ot.evalCtx.SessionData().PropagateInputOrdering = ot.Flags.PropagateInputOrdering
	ot.evalCtx.SessionData().NullOrderedLast = ot.Flags.NullOrderedLast
	ot.evalCtx.SessionData().OptimizerUseMultiColStats = ot.Flags.UseMultiColStats

That seems like too manual of a method. Wouldn't it be easier to support setting any session flag through a single opttest flag instead of manually instrumenting each one? This would also speed up test development.

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: I don't see the same nil checks in other places, are you sure it's necessary?

Also, you can access the eval context from c.e.evalCtx.

Was just trying to be safe. OK, removed the nil pointer check and modified to use c.e.evalCtx instead of c.e.f.EvalContext(). What's the benefit of using c.e.evalCtx ?

Code quote:

CanHoistProjectInput

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 574 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

That seems like too manual of a method. Wouldn't it be easier to support setting any session flag through a single opttest flag instead of manually instrumenting each one? This would also speed up test development.

@mgartner
The set opttest flag currently behaves differently than ot.Flags. Those persist, but set is currently only in effect for a single statement. If we're going to get rid of the old pattern of setting session flags in opt tests, would we want to make the new method persist the settings, or reset after each statement? I can think of pros and cons of both behaviors.

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @msirek)

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 574 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

@mgartner
The set opttest flag currently behaves differently than ot.Flags. Those persist, but set is currently only in effect for a single statement. If we're going to get rid of the old pattern of setting session flags in opt tests, would we want to make the new method persist the settings, or reset after each statement? I can think of pros and cons of both behaviors.

nvm, ot.Flags only affects the current statement. I've removed all of the session flags in OptTester.Flags so new session flags uses in opt tests useset.

[cucaroach]

Reviewed 6 of 8 files at r1, 4 of 4 files at r3, 4 of 4 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @msirek)

---

-- commits line 2 at r1:

Previously, msirek (Mark Sirek) wrote…

By title, do you mean the PR title or this text line? I've updated the PR title.

I think he's referring to the first line of the commit message.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 911 at r4 (raw file):

				return errors.Errorf("Expected both session variable name and value for set command")
			}
			err := sql.SetSessionVariable(f.ot.ctx, f.ot.evalCtx, s[0], s[1])

Does opttester already depend on sql? I think it doesn't looking at go list -f '{{ .Imports }}' ./pkg/sql/opt/testutils/opttester. So I think this means if sql changes opttester has to recompile. Not a big deal I think but thought I'd call it out in case anyone thinks it is a big deal. I wonder if SetSessionVariable should live in a smaller downstream package like pkg/sql/sessiondata.

[DrewKimball]

once my comments have been resolved.

Reviewed 6 of 8 files at r1, 4 of 4 files at r3, 4 of 4 files at r4, 35 of 35 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, @mgartner, and @msirek)

---

pkg/sql/vars.go line 2017 at r3 (raw file):

		GlobalDefault: globalFalse,
	},
	// CockroachDB extension.

[nit] lost a newline above here.

---

pkg/sql/vars.go line 2190 at r4 (raw file):

// SetSessionVariable sets a new value for session setting `varName` is the
// session settings owned by `evalCtx`, returning an error if not successful.
func SetSessionVariable(

I think instead of adding this you might be able to use the existing evalCtx.SessionAccessor.SetSessionVar()

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 911 at r4 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Does opttester already depend on sql? I think it doesn't looking at go list -f '{{ .Imports }}' ./pkg/sql/opt/testutils/opttester. So I think this means if sql changes opttester has to recompile. Not a big deal I think but thought I'd call it out in case anyone thinks it is a big deal. I wonder if SetSessionVariable should live in a smaller downstream package like pkg/sql/sessiondata.

Could use f.ot.evalCtx.SessionAccessor.SetSessionVar here (see above).

[msirek]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, and @mgartner)

---

-- commits line 2 at r1:

Previously, cucaroach (Tommy Reilly) wrote…

I think he's referring to the first line of the commit message.

OK, thanks. Updated the commit message.

---

pkg/sql/vars.go line 2017 at r3 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] lost a newline above here.

Fixed

Code quote:

show_primary_key_constraint_on_not_visible_columns

---

pkg/sql/vars.go line 2190 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I think instead of adding this you might be able to use the existing evalCtx.SessionAccessor.SetSessionVar()

Thanks. I tried this, but evalCtx.SessionAccessor (which typically points to a planner) is nil in opt tests. I'm not sure what the implications of going through the extra setup steps to set up a planner (and any other standard initializations; many things may need to be set up) would be. I'm hoping to avoid introducing more overhead (CPU or memory) in opt tests (many of them don't even use this new flag).

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 574 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

The current pattern is to set the session variable here rather than introducing the SetSessionVariable function.

see comments below

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 911 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Could use f.ot.evalCtx.SessionAccessor.SetSessionVar here (see above).

SetSessionVariable references sessionDataMutatorBase and sessionDataMutator, which also live in sql. Unless these items are also moved to pkg/sql/sessiondata, moving SetSessionVariable there would cause a circular dependency I think.

[mgartner]

Cool!

Can the set flag set multiple session settings for the same test? Might be good to document how to do that if the syntax is not obvious.

Reviewed 1 of 4 files at r3, 37 of 37 files at r6, 4 of 4 files at r7, 35 of 35 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @DrewKimball, @mgartner, and @msirek)

---

pkg/sql/vars.go line 2189 at r7 (raw file):

// SetSessionVariable sets a new value for session setting `varName` is the
// session settings owned by `evalCtx`, returning an error if not successful.

nit: Leave a note that this should only be used for testing, and that SessionAccessor.SetSessionVar should be preferred.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 909 at r7 (raw file):

			s := strings.Split(val, "=")
			if len(s) != 2 {
				return errors.Errorf("Expected both session variable name and value for set command")

nit: "set command" => "set flag"

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 513 at r8 (raw file):

//  - join-limit: sets the value for SessionData.ReorderJoinsLimit, which
//    indicates the number of joins at which the optimizer should stop
//    attempting to reorder.

nit: You should document set somewhere in this list.

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

Was just trying to be safe. OK, removed the nil pointer check and modified to use c.e.evalCtx instead of c.e.f.EvalContext(). What's the benefit of using c.e.evalCtx ?

Not a big difference, but it's shorter and it might avoid a function call (unless EvalContext() is inlined by the compiler).

[mgartner]

@msirek it'd be ideal to get this PR and backports merged today so that the fixes can make it into the next 22.1 and 21.2 releases. The most pressing is the 21.2 backport.

[msirek]

Thanks, I'll try to get this merged ASAP.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @mgartner)

---

pkg/sql/vars.go line 2189 at r7 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: Leave a note that this should only be used for testing, and that SessionAccessor.SetSessionVar should be preferred.

Done

Code quote (from pkg/sql/opt/testutils/opttester/opt_tester.go):

inject-stats: the file path is relative to the test file.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 909 at r7 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: "set command" => "set flag"

Fixed

Code quote:

Expected both session variable name and value for set command")

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 513 at r8 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: You should document set somewhere in this list.

Done

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Not a big difference, but it's shorter and it might avoid a function call (unless EvalContext() is inlined by the compiler).

OK, thanks.

[mgartner]

Reviewed 43 of 43 files at r9, 4 of 4 files at r10, 35 of 35 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @DrewKimball, @mgartner, and @msirek)

---

-- commits line 20 at r12:
nit: squash this commit into the previous commit

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 286 at r10 (raw file):

		evalCtx: eval.MakeTestingEvalContext(cluster.MakeTestingClusterSettings()),
	}
	ot.Flags.ot = ot

nit: separate ctx and evalCtx fields would be less awkward than this reference cycle. Another option is to pass these into Flags.Set as arguments.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 260 at r11 (raw file):

	ctx := context.Background()
	ot := &OptTester{
		Flags:   Flags{},

nit: you can remove this line now

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

OK, thanks.

I should have also mentioned: within the context of xform.CustomFuncs, it's more natural to reach to the eval context in c.e *explorer (explorer is part of the xform package) than to reach for the eval context in c.e.f *norm.Factory. That's probably the main reason I'd use c.e.evalCtx in a situation like this instead of c.e.f.EvalContext().

[msirek]

Can the set flag set multiple session settings for the same test? Might be good to document how to do that if the syntax is not obvious.

Yes. The syntax is the same as for other flags. These are space-separated flags, e.g.

opt expect=HoistProjectFromInnerJoin set=disable_hoist_projection_in_join_limitation=true set=prefer_lookup_joins_for_fks=true

[msirek]

TFTRs!
bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @mgartner)

---

-- commits line 20 at r12:

Previously, mgartner (Marcus Gartner) wrote…

nit: squash this commit into the previous commit

I did fixup on it and squashed all other commits. This was a temporary commit to aid in cherry-picking to backport branches.

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 286 at r10 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: separate ctx and evalCtx fields would be less awkward than this reference cycle. Another option is to pass these into Flags.Set as arguments.

OK, I picked the first option. I have seen reference cycles elsewhere. For example, norm.Factory -> funcs (norm.CustomFuncs) -> f(norm.Factory). Do you think we should open an issue to remove this and other reference cycles?

Code quote:

ot.Flags.ot = ot

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 260 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: you can remove this line now

Done

---

pkg/sql/opt/xform/join_funcs.go line 1687 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I should have also mentioned: within the context of xform.CustomFuncs, it's more natural to reach to the eval context in c.e *explorer (explorer is part of the xform package) than to reach for the eval context in c.e.f *norm.Factory. That's probably the main reason I'd use c.e.evalCtx in a situation like this instead of c.e.f.EvalContext().

OK, thanks.

[msirek]

bors r-

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @mgartner)

[craig]

Canceled.

[msirek]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @mgartner)

[mgartner]

pkg/sql/opt/testutils/opttester/opt_tester.go line 286 at r10 (raw file):

Previously, msirek (Mark Sirek) wrote…

OK, I picked the first option. I have seen reference cycles elsewhere. For example, norm.Factory -> funcs (norm.CustomFuncs) -> f(norm.Factory). Do you think we should open an issue to remove this and other reference cycles?

It's probably not necessary, just something to avoid to prevent if possible to avoid confusion. I don't feel to strongly about it. The norm.Factory norm.CustomFuncs case it required because they depend on each other - this case was different because Flags didn't depend on ot, only two fields within ot.

[msirek]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[msirek]

bors r+

[craig]

Already running a review

[msirek]

bors r+

[craig]

Already running a review

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[msirek]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[msirek]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)