norm: update prune cols to match PruneJoinLeftCols/PruneJoinRightCols

[msirek]

In #90599 adjustments where made to the PruneJoinLeftCols and PruneJoinRightCols
normalization rules to avoid pruning columns which might be needed when
deriving new predicates based on foreign key constraints for lookup join.

However, this caused a problem where rules might sometimes fire in an
infinite loop because the same columns to prune keep getting added as
PruneCols in calls to DerivePruneCols. The logic in prune_cols.opt and
DerivePruneCols must be kept in sync to avoid such problems, and this
PR brings it back in sync.

Epic: none
Fixes: #100478

Release note: None

[cockroach-teamcity]

This change is 

[DrewKimball]

I think you can also remove the calls to AddDerivedOnClauseConditionsFromFKContraints in prune_cols.opt, since the change to DerivePruneCols should prevent the columns from being pruned as desired.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained

[msirek]

Thanks. I tried backing out the call in prune_cols.opt, but then the derived terms didn't get applied:

    optimizer_test.go:256: 
        /Users/msirek/go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/xform/testdata/rules/join:13133:
        opt [1 args]
        SELECT *
        FROM t69617_uniq_fk_child
        JOIN LATERAL (SELECT * FROM t69617_uniq_fk_parent WHERE t69617_uniq_fk_child.b = t69617_uniq_fk_parent.b)
        ON true WHERE b = 1
        ----
        inner-join (lookup t69617_uniq_fk_parent@t69617_uniq_fk_parent_a_b_c_key)
         ├── columns: a:1!null b:2!null c:3!null a:7!null b:8!null c:9!null
         ├── key columns: [1 2 3] = [7 8 9]
         ├── lookup columns are key
         ├── fd: ()-->(2,7-9), (2)==(8), (8)==(2)
         ├── select
         │    ├── columns: t69617_uniq_fk_child.a:1!null t69617_uniq_fk_child.b:2!null t69617_uniq_fk_child.c:3!null
         │    ├── fd: ()-->(2)
         │    ├── scan t69617_uniq_fk_child
         │    │    └── columns: t69617_uniq_fk_child.a:1!null t69617_uniq_fk_child.b:2!null t69617_uniq_fk_child.c:3!null
         │    └── filters
         │         └── t69617_uniq_fk_child.b:2 = 1 [outer=(2), constraints=(/2: [/1 - /1]; tight), fd=()-->(2)]
         └── filters
              └── t69617_uniq_fk_parent.b:8 = 1 [outer=(8), constraints=(/8: [/1 - /1]; tight), fd=()-->(8)]
    opt_tester.go:849: /Users/msirek/go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/xform/testdata/rules/join:13155: expected to see GenerateLookupJoins, but was not triggered. Did see CommuteRightJoin, EliminateProject, PruneJoinLeftCols, PruneJoinRightCols, CommuteLeftJoin, GenerateIndexScans

Maybe this is because the PruneJoinLeftCols and PruneJoinRightCols aren't using PruneCols directly but calculating them separately for the left and right using the ON clause OuterCols, while relProps.Rule.PruneCols (from DerivePruneCols) has all pruneable columns from both the left and the right:

cockroach/pkg/sql/opt/norm/prune_cols_funcs.go

Line 584 in 6ff73d6

relProps.Rule.PruneCols = leftPruneCols.Union(rightPruneCols)

I think in order to be able to remove the call to AddDerivedOnClauseConditionsFromFKContraints in prune_cols.opt we'd have to also update OuterCols to include the columns from the conditions which haven't yet been derived/applied. But I don't think we want to do that.

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

[DrewKimball]

Maybe this is because the PruneJoinLeftCols and PruneJoinRightCols aren't using PruneCols directly but calculating them separately for the left and right using the ON clause OuterCols

That makes sense, I think you're right. In that case we could change $needed to be:

$needed:(DifferenceCols (OutputCols $input) (DerivePruneCols $input))

and that way avoid duplicate effort. I think the only reason this wasn't done before was probably to keep from defining DerivePruneCols on CustomFuncs, and we've done that now anyway.

[mgartner]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek)

---

-- commits line 4 at r2:
nit: The first line of this paragraph is missing because it starts with #, which is considered a comment in a git commit message. You could reword to something like: "In #90599 adjustments where made ..."

---

pkg/sql/opt/norm/testdata/rules/prune_cols line 5316 at r2 (raw file):

# A join which doesn't prune columns which could potentially appear in derived
# ON clause conditions should not result in infinite rule recursion.
norm expect=EliminateGroupByProject expect=PruneJoinLeftCols expect=PruneJoinRightCols

nit: You can put all the expects together:

norm expect=(EliminateGroupByProject,PruneJoinLeftCols,PruneJoinRightCols)

[msirek]

I believe that expression might not be equivalent to the original. In the rules, we try to figure out only the set of columns which are actually needed. For example, in PruneJoinRightCols, UnionCols3 is looking at the passthrough, projection and filter columns of the join operation itself while OutputCols of the join is the full set of output columns from the left and right inputs to the join:

cockroach/pkg/sql/opt/memo/logical_props_builder.go

Lines 2401 to 2402 in 22dd13c

	default:
	cols = h.leftProps.OutputCols.Union(h.rightProps.OutputCols)

I believe some of these columns could still be projected away if they are not present in the passthrough and projection columns of the join.

And, DerivePruneCols seems to find all columns which could be candidates for pruning, not the final set of columns which can be pruned, so, removing all of the candidate pruning columns from $needed might mistakenly prune all candidate columns, some of which may be present in passthrough or projection columns of the join operation.

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

---

-- commits line 4 at r2:

Previously, mgartner (Marcus Gartner) wrote…

nit: The first line of this paragraph is missing because it starts with #, which is considered a comment in a git commit message. You could reword to something like: "In #90599 adjustments where made ..."

Fixed

---

pkg/sql/opt/norm/testdata/rules/prune_cols line 5316 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: You can put all the expects together:

norm expect=(EliminateGroupByProject,PruneJoinLeftCols,PruneJoinRightCols)

Done

[DrewKimball]

I believe that expression might not be equivalent to the original. In the rules, we try to figure out only the set of columns which are actually needed. For example, in PruneJoinRightCols, UnionCols3 is looking at the passthrough, projection and filter columns of the join operation itself while OutputCols of the join is the full set of output columns from the left and right inputs to the join

Alright, let's just keep things as-is, then.

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

---

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

		return nil, err
	}
	o.Memo().ResetLogProps(ot.ctx, &ot.evalCtx)

Why this change?

[msirek]

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

---

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

Why this change?

Because Optimize clears out the logPropsBuilder here:

cockroach/pkg/sql/opt/memo/memo.go

Line 293 in 57dacce

m.logPropsBuilder.clear()

And the after the opt tester calls Optimize it calls to postProcess -> fillInLazyProps -> c.DerivePruneCols -> c.AddDerivedOnClauseConditionsFromFKContraints ... and may need to access logPropsBuilder.mem. If it's nil, we hit a segmentation violation (which I hit in xform/rules/join).

[msirek]

TFTRs!
bors r+

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[mgartner]

blathers backport 23.1.0