xform: derive implicit predicates from FK constraint for lookup join

[msirek]

Fixes #69617

When a unique constraint exists on a subset of the referenced columns in
a foreign key constraint, the remaining columns in the constraint can be
used to generate equijoin predicates which may enable more efficient use
of an index on the lookup side of a lookup join. If the index is a
multiregion index, a join predicate may be derived which could
potentially eliminate reads of remote rows.

Example:

CREATE TABLE accounts (
    account_id  UUID PRIMARY KEY DEFAULT gen_random_uuid(),
    name        STRING NOT NULL,
    crdb_region crdb_internal_region NOT NULL,
    UNIQUE INDEX acct_id_crdb_region_idx (account_id, crdb_region)
) LOCALITY GLOBAL;

drop table if exists tweets;
CREATE TABLE tweets (
    account_id UUID NOT NULL,
    tweet_id   UUID DEFAULT gen_random_uuid(),
    message    STRING NOT NULL,
    crdb_region crdb_internal_region NOT NULL,
    PRIMARY KEY (crdb_region, account_id, tweet_id),
    -- The PK of accounts is a subset of the referenced columns in
    -- the FK constraint.
    FOREIGN KEY (account_id, crdb_region)
    REFERENCES accounts (account_id, crdb_region)
    ON DELETE CASCADE ON UPDATE CASCADE
) LOCALITY REGIONAL BY ROW as crdb_region;

-- Join on account_id uses the uniqueness of accounts_pkey and the FK
-- constraint to derive tweets.crdb_region = accounts.crdb_region
EXPLAIN SELECT *
    FROM tweets
    INNER LOOKUP JOIN accounts@acct_id_crdb_region_idx USING (account_id)
WHERE account_id = '6f781502-4936-43cc-b384-04e5cf292cc8';
-------------------------------------------------------------------------
  distribution: local
  vectorized: true

  • lookup join
  │ table: accounts@accounts_pkey
  │ equality: (account_id) = (account_id)
  │ equality cols are key
  │
  └── • lookup join
      │ table: accounts@acct_id_crdb_region_idx
      │ equality: (account_id, crdb_region) = (account_id,crdb_region)
      │ equality cols are key
      │ pred: account_id = '6f781502-4936-43cc-b384-04e5cf292cc8'
      │
      └── • scan
            missing stats
            table: tweets@tweets_pkey
            spans: [/'ca'/'6f781502-4936-43cc-b384-04e5cf292cc8' - /'ca'/'6f781502-4936-43cc-b384-04e5cf292cc8'] [/'eu'/'6f781502-4936-43cc-b384-04e5cf292cc8' - /'eu'/'6f781502-4936-43cc-b384-04e5cf292cc8'] [/'us'/'6f781502-4936-43cc-b384-04e5cf292cc8' - /'us'/'6f781502-4936-43cc-b384-04e5cf292cc8']

Release note (performance improvement): This patch enables more
efficient lookup joins by deriving new join constraints when
equijoin predicates exist on the column(s) of a unique constraint on one
table which are a proper subset of the referencing columns of a foreign
key constraint on the other table. If an index exists on those FK
constraint referencing columns, equijoin predicates are derived between
the PK and FK columns not currently bound by ON clause predicates.

norm: do not prune scan columns which may show up in derived join terms

Fixes #69617

This commit amends the PruneJoinLeftCols and PruneJoinRightCols
normalization rules to include potential derived ON clause predicates so
that columns not present in the SELECT list are not pruned away from the
involved Scans before predicates are derived. Derived ON clause
predicate columns are excluded from the set of columns to use for
equijoin selectivity estimation.

Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

Nice work! There is definitely a lot of subtlety here...

Before you merge this and auto-close #69617, can you make sure to open another issue as described here? #69617 (comment)

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

---

pkg/sql/opt/lookupjoin/constraint_builder.go line 203 at r2 (raw file):

	// terms to evaluate. Also, that approach may cause selectivity
	// underestimation, so would require more effort to make sure correlations
	// between columns are accurately captured.

This is kind of a shame. I think this makes sense for this PR, but in the future it would be great to move most of the logic from this PR into logical_props_builder.go to improve the FuncDeps for the join and include these equivalencies. We should improve the logic in statistics_builder so that it doesn't underestimate selectivity.

---

pkg/sql/opt/norm/join_funcs.go line 655 at r1 (raw file):

	if !ok {
		return nil
	}

Seems like you shouldn't need to restrict to just these operators. Can't you just look at the output columns and find the corresponding base table using md.ColumnMeta(col).Table (if non-zero)?

I also feel like this function may be missing some important logic, and should be doing something similar to what we're doing here:

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

Line 485 in dd7bdf4

func checkForeignKeyCase(

Notice that function has some checks that you don't have here, such as tableMeta.IgnoreForeignKeys, !fk.Validated(), and fk.MatchMethod() != tree.MatchFull (among other things...)

---

pkg/sql/opt/norm/join_funcs.go line 666 at r1 (raw file):

		pkFkFilters = nil
		nextFK := false
		var pkColSet opt.ColSet

this might not be a pk -- could be any key

---

pkg/sql/opt/norm/join_funcs.go line 638 at r2 (raw file):

	maybeFKTableScanExpr memo.RelExpr,
	pkTableScanPrivate *memo.ScanPrivate,
	indexCols, onClauseLookupRelStrictKeyCols, lookupRelEquijoinCols opt.ColSet,

nit: These names seem overly complex and also slightly misleading. For example, I don't think we require that this is actually a pk-fk join, simply that there is a foreign key join. Instead of maybeFKTableScanExpr and pkTableScanPrivate can you just call them input and lookupTableScanPrivate or something like that?

And instead of onClauseLookupRelStrictKeyCols... something a bit more concise?

---

pkg/sql/opt/norm/join_funcs.go line 757 at r2 (raw file):

	if !ok {
		return on
	}

Similar to what I wrote above, seems like you could look at the output columns and find the corresponding base table using md.ColumnMeta(col).Table (if non-zero)

---

pkg/sql/opt/norm/join_funcs.go line 790 at r2 (raw file):

func (c *CustomFuncs) findAdditionalJoinColsFromFKContraints(
	lookupTable *memo.ScanExpr, on memo.FiltersExpr,
) (keyCols, joinCols opt.ColSet, ok bool) {

This function looks like it's nearly identical to another one you added in this PR, which also seemed to be possibly duplicating some logic. Can you try to reuse existing code?

---

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

// contain a strict key on lookupTableScanPrivate, the key columns and entire
// set of equijoin columns are returned.
func (c *CustomFuncs) getOnClauseLookupRelStrictKeyCols(

feels like this function is repeating a lot of the logic already performed inside ConstraintBuilder.Build, which uses memo.ExtractJoinEqualityColumnsWithFilterOrds. I wonder if you could move some of this logic inside ConstraintBuilder.Build and remove this function?

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_foreign_key_lookup_join line 5 at r1 (raw file):

# Set the closed timestamp interval to be short to shorten the amount of time
# we need to wait for the system config to propagate.

Will the test be flaky without this? Or just slower? If flaky, are you sure this fixes it?

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_foreign_key_lookup_join line 85 at r1 (raw file):

# Verify inverted index cases cannot derive a 'cr = cr' condition. Lookup join
# is not possible.
statement error could not produce a query plan conforming to the LOOKUP JOIN hint

What if you hint inverted join instead?

[msirek]

Thanks. Created #91084.

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

---

pkg/sql/opt/lookupjoin/constraint_builder.go line 203 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

This is kind of a shame. I think this makes sense for this PR, but in the future it would be great to move most of the logic from this PR into logical_props_builder.go to improve the FuncDeps for the join and include these equivalencies. We should improve the logic in statistics_builder so that it doesn't underestimate selectivity.

I agree. I'm just afraid implicitly adding more predicates will change a lot of query plans. Joining on multiple columns tends to drop the estimated row count towards 1, which is more like a independence assumption. We should be doing better when the join predicates overlap with an FK constraint. I've opened #91142 to improve our estimates when an FK constraint is present.

---

pkg/sql/opt/norm/join_funcs.go line 655 at r1 (raw file):

Seems like you shouldn't need to restrict to just these operators.

Added parameter fkChildEquijoinCols and use of ColumnMeta to find the applicable base table(s).

I also feel like this function may be missing some important logic, and should be doing something similar to what we're doing here:

Added IgnoreForeignKeys, Validated andMatchMethod checks. I did not add a if fkChildTable.Column(fkChildColumnOrd).IsNullable() check, like in checkForeignKeyCase, to allow cases where the output column is nullable but the base table column is not because I could not find a test case. Even if the column is from the outer table of outer join, it is included in fkChild.Relational().NotNullCols if the base table column is NOT NULL. I also did not add an UnfilteredCols check, because it is OK if we filter rows out of the lookup table.

---

pkg/sql/opt/norm/join_funcs.go line 666 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

this might not be a pk -- could be any key

Dropped the use of 'pk' in names in favor of differentiating the tables with names including 'child' and 'parent'

Code quote:

for i := 0; i < fkTable.OutboundForeignKeyCount

---

pkg/sql/opt/norm/join_funcs.go line 638 at r2 (raw file):

can you just call them input and lookupTableScanPrivate or something like that?

I would like to capture in the names which parameter is the foreign key child table and which one is the parent table. Changed to fkChild and fkParentScanPrivate.

And instead of onClauseLookupRelStrictKeyCols... something a bit more concise?

Changed to fkParentJoinKey

---

pkg/sql/opt/norm/join_funcs.go line 757 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Similar to what I wrote above, seems like you could look at the output columns and find the corresponding base table using md.ColumnMeta(col).Table (if non-zero)

Updated ForeignKeyConstraintFilters to look up tables by ColumnID.

Code quote:

nextFK = true

---

pkg/sql/opt/norm/join_funcs.go line 790 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

This function looks like it's nearly identical to another one you added in this PR, which also seemed to be possibly duplicating some logic. Can you try to reuse existing code?

Created norm.GetJoinKeyAndEquijoinCols for duplicated code. I don't see a whole lot of duplicated code in ForeignKeyConstraintFilters in its current iteration.

Code quote:

.ReferencedColumnOrdinal(

---

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

Previously, rytaft (Rebecca Taft) wrote…

feels like this function is repeating a lot of the logic already performed inside ConstraintBuilder.Build, which uses memo.ExtractJoinEqualityColumnsWithFilterOrds. I wonder if you could move some of this logic inside ConstraintBuilder.Build and remove this function?

Updated getOnClauseLookupRelStrictKeyCols and findAdditionalJoinColsFromFKContraints to callExtractJoinEqualityColumns. I'm not sure if it would help much further to push this logic into ConstraintBuilder.Build. If the end goal is to build these constraints for all joins, not just lookup joins, then it may be good to keep the logic separated so it will be easy to transplant elsewhere. What do you think?

Code quote:

getOnClauseLookupRelStrictKeyCols

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_foreign_key_lookup_join line 5 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Will the test be flaky without this? Or just slower? If flaky, are you sure this fixes it?

This reduces the likelihood of flakes, but doesn't prevent them (I found some running this under --stress). I've added the necessary retrys.

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_foreign_key_lookup_join line 85 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

What if you hint inverted join instead?

Done

Code quote:

Verify inverted index cases cannot derive a 'cr = cr' condition.

[rytaft]

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

---

pkg/sql/opt/lookupjoin/constraint_builder.go line 203 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

I agree. I'm just afraid implicitly adding more predicates will change a lot of query plans. Joining on multiple columns tends to drop the estimated row count towards 1, which is more like a independence assumption. We should be doing better when the join predicates overlap with an FK constraint. I've opened #91142 to improve our estimates when an FK constraint is present.

Thanks!

---

pkg/sql/opt/norm/join_funcs.go line 757 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

Updated ForeignKeyConstraintFilters to look up tables by ColumnID.

But why can't you also change it here? Why do you need to check for project / select / scan?

---

pkg/sql/opt/norm/join_funcs.go line 790 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

Created norm.GetJoinKeyAndEquijoinCols for duplicated code. I don't see a whole lot of duplicated code in ForeignKeyConstraintFilters in its current iteration.

Still looks nearly identical to getOnClauseLookupRelStrictKeyCols. I see that one takes a ScanExpr while one takes a ScanPrivate, but otherwise the logic is basically the same. I think it would be better if you had one function with all the relevant logic.

I also think it's confusing that this function references FK constraints, since nothing about the logic actually requires that there are FK constraints. I think it would be better if the function name describes more clearly what it does (instead of perhaps what the caller is doing).

---

pkg/sql/opt/norm/join_funcs.go line 646 at r3 (raw file):

	fkChildEquijoinColSet := fkChildEquijoinCols.ToSet()

	tableIDs := make(map[opt.TableID]struct{})

nit: you could use a FastIntSet to avoid allocations here

---

pkg/sql/opt/norm/join_funcs.go line 654 at r3 (raw file):

	})

	matchedEquijoinCols := make(map[opt.ColumnID]opt.ColumnID)

nit: use opt.ColMap

---

pkg/sql/opt/norm/join_funcs.go line 683 at r3 (raw file):

			}
			fkFilters = nil
			nextFK := false

nit: nextFK isn't very descriptive. Consider initializing this to true and caling it fkFiltersValid or something like that instead.

---

pkg/sql/opt/norm/join_funcs.go line 880 at r3 (raw file):

		childTableJoinCols = make(opt.ColList, 0, reducedJoinKeyCols.Len())
		parentTableJoinCols = make(opt.ColList, 0, reducedJoinKeyCols.Len())
		for i, lookupColID := range parentJoinCols {

nit: lookupColID -> parentJoinCol ?

---

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

Previously, msirek (Mark Sirek) wrote…

Updated getOnClauseLookupRelStrictKeyCols and findAdditionalJoinColsFromFKContraints to callExtractJoinEqualityColumns. I'm not sure if it would help much further to push this logic into ConstraintBuilder.Build. If the end goal is to build these constraints for all joins, not just lookup joins, then it may be good to keep the logic separated so it will be easy to transplant elsewhere. What do you think?

Sounds reasonable.

---

pkg/sql/opt/xform/testdata/rules/join line 13039 at r3 (raw file):

# Outer join as the input to lookup join may derive predicates from the FK constraint.
opt

Consider using an expect assertion (here and elsewhere in this file)

---

pkg/sql/opt/xform/testdata/rules/join line 13066 at r3 (raw file):

# The optimizer may not derive predicates on nullable columns from an FK
# constraint which does not use the MATCH FULL option.
opt

Consider using an expect-not assertion

[msirek]

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

---

pkg/sql/opt/norm/join_funcs.go line 757 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

But why can't you also change it here? Why do you need to check for project / select / scan?

This case is different. The other case dealt with the input side of lookup join. This case deals with the lookup side of lookup join, and I want to be able to access the actual ScanPrivate.Cols included in the ScanExpr so we know the exact cases that can extract join equality columns. Note that GenerateLookupJoins is only called when the lookup relation is a ScanExpr, or when it's a ScanExpr nested under a SelectExpr (via rule GenerateLookupJoinsWithFilter). I suppose peeking under a ProjectExpr is not required, so I've removed that case.

Code quote:

if !leftScanFound && !rightScanFound {

---

pkg/sql/opt/norm/join_funcs.go line 790 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Still looks nearly identical to getOnClauseLookupRelStrictKeyCols. I see that one takes a ScanExpr while one takes a ScanPrivate, but otherwise the logic is basically the same. I think it would be better if you had one function with all the relevant logic.

I also think it's confusing that this function references FK constraints, since nothing about the logic actually requires that there are FK constraints. I think it would be better if the function name describes more clearly what it does (instead of perhaps what the caller is doing).

OK. Moved getOnClauseLookupRelStrictKeyCols to norm and renamed it to GetEquijoinStrictKeyCols so no mention of FKs or lookup joins is present. Previous calls to findAdditionalJoinColsFromFKContraints call this other function.

---

pkg/sql/opt/norm/join_funcs.go line 646 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: you could use a FastIntSet to avoid allocations here

I wrote a couple quick microbenchmarks for this. I didn't see any allocations happening with the map, and FastIntSet was a lot slower:

func BenchmarkFastIntSetMem(b *testing.B) {
	tableIDs := []int{12345678993, 82345678993, 62345678993}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		set := FastIntSet{}
		set.Add(tableIDs[0])
		set.Add(tableIDs[1])
		set.Add(tableIDs[2])
	}
	b.StopTimer()
}

func BenchmarkMapMem(b *testing.B) {
	tableIDs := []opt.TableID{12345678993, 82345678993, 62345678993}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		tableIDMap := make(map[opt.TableID]struct{})
		tableIDMap[tableIDs[0]] = struct{}{}
		tableIDMap[tableIDs[1]] = struct{}{}
		tableIDMap[tableIDs[2]] = struct{}{}
	}
	b.StopTimer()
}

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkMapMem-32      1000000000               0.08839 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08444 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08670 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07948 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07415 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07491 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08621 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08740 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08298 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07532 ns/op         0 B/op          0 allocs/op

make bench PKG=./pkg/util BENCHES=BenchmarkFastIntSetMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkFastIntSetMem-32       1000000000               0.6314 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6341 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6222 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6272 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6746 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6580 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6636 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.5957 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6456 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6900 ns/op          0 B/op          0 allocs/op

---

pkg/sql/opt/norm/join_funcs.go line 653 at r3 (raw file):

		}
	})

---

pkg/sql/opt/norm/join_funcs.go line 654 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: use opt.ColMap

I benchmarked this as well and found opt.ColMap (util.FastIntMap) to be slower than a regular map and perform allocations, while the regular map does not.

func BenchmarkColumnIDMapMem(b *testing.B) {
	colID1 := []opt.ColumnID{1, 9, 100, 300, 2000, 10000}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		colMap := make(map[opt.ColumnID]opt.ColumnID)
		colMap[colID1[0]] = colID1[0]
		colMap[colID1[1]] = colID1[1]
		colMap[colID1[2]] = colID1[2]
		colMap[colID1[3]] = colID1[3]
		colMap[colID1[4]] = colID1[4]
		colMap[colID1[5]] = colID1[5]
	}
	b.StopTimer()
}

func BenchmarkFastIntMapMem(b *testing.B) {
	colID1 := []int{1, 9, 100, 300, 2000, 10000}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		colMap := util.FastIntMap{}
		colMap.Set(colID1[0], colID1[0])
		colMap.Set(colID1[1], colID1[1])
		colMap.Set(colID1[2], colID1[2])
		colMap.Set(colID1[3], colID1[3])
		colMap.Set(colID1[4], colID1[4])
		colMap.Set(colID1[5], colID1[5])
	}
	b.StopTimer()
}

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkFastIntMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkFastIntMapMem-32              1        1314439680 ns/op        1200006768 B/op  2000021 allocs/op
BenchmarkFastIntMapMem-32              1        1250450352 ns/op        1200000224 B/op  2000006 allocs/op
BenchmarkFastIntMapMem-32              1        1255882487 ns/op        1200000792 B/op  2000011 allocs/op
BenchmarkFastIntMapMem-32              1        1277546920 ns/op        1200000248 B/op  2000006 allocs/op
BenchmarkFastIntMapMem-32              1        1313803530 ns/op        1200000128 B/op  2000005 allocs/op
BenchmarkFastIntMapMem-32              1        1221278816 ns/op        1200000208 B/op  2000004 allocs/op
BenchmarkFastIntMapMem-32              1        1249782485 ns/op        1200000288 B/op  2000003 allocs/op
BenchmarkFastIntMapMem-32              1        1285993783 ns/op        1200000304 B/op  2000005 allocs/op
BenchmarkFastIntMapMem-32              1        1250126388 ns/op        1200000192 B/op  2000002 allocs/op
BenchmarkFastIntMapMem-32              1        1352475944 ns/op        1200000408 B/op  2000007 allocs/op

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkColumnIDMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkColumnIDMapMem-32      1000000000               0.1788 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1738 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1713 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1678 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1823 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1819 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1860 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1808 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1714 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1775 ns/op          0 B/op          0 allocs/op

I think since the maps are only used local to this function, Go is smart enough to only use stack memory for them.

---

pkg/sql/opt/norm/join_funcs.go line 683 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: nextFK isn't very descriptive. Consider initializing this to true and caling it fkFiltersValid or something like that instead.

Done

---

pkg/sql/opt/norm/join_funcs.go line 880 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: lookupColID -> parentJoinCol ?

Since we're eliminating references to parent and child tables of the FK constraint (which as you pointed out isn't even used here), I've renamed these to use table and inputRel prefixes.

---

pkg/sql/opt/xform/testdata/rules/join line 13039 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Consider using an expect assertion (here and elsewhere in this file)

I'm not sure which rule to assert. GenerateLookupJoins is triggered whether or not any lookup joins are actually built.

Code quote:

Outer join as the input to lookup join may derive predicates from the FK constraint.

---

pkg/sql/opt/xform/testdata/rules/join line 13066 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Consider using an expect-not assertion

Same as above. ForeignKeyConstraintFilters is not conditionally triggered by any rule. Which rule were you thinking should be tested here?

[rytaft]

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

---

pkg/sql/opt/norm/join_funcs.go line 646 at r3 (raw file):

Previously, msirek (Mark Sirek) wrote…

I wrote a couple quick microbenchmarks for this. I didn't see any allocations happening with the map, and FastIntSet was a lot slower:

func BenchmarkFastIntSetMem(b *testing.B) {
	tableIDs := []int{12345678993, 82345678993, 62345678993}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		set := FastIntSet{}
		set.Add(tableIDs[0])
		set.Add(tableIDs[1])
		set.Add(tableIDs[2])
	}
	b.StopTimer()
}

func BenchmarkMapMem(b *testing.B) {
	tableIDs := []opt.TableID{12345678993, 82345678993, 62345678993}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		tableIDMap := make(map[opt.TableID]struct{})
		tableIDMap[tableIDs[0]] = struct{}{}
		tableIDMap[tableIDs[1]] = struct{}{}
		tableIDMap[tableIDs[2]] = struct{}{}
	}
	b.StopTimer()
}

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkMapMem-32      1000000000               0.08839 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08444 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08670 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07948 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07415 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07491 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08621 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08740 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.08298 ns/op         0 B/op          0 allocs/op
BenchmarkMapMem-32      1000000000               0.07532 ns/op         0 B/op          0 allocs/op

make bench PKG=./pkg/util BENCHES=BenchmarkFastIntSetMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkFastIntSetMem-32       1000000000               0.6314 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6341 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6222 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6272 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6746 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6580 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6636 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.5957 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6456 ns/op          0 B/op          0 allocs/op
BenchmarkFastIntSetMem-32       1000000000               0.6900 ns/op          0 B/op          0 allocs/op

Thanks for running the benchmark!

---

pkg/sql/opt/norm/join_funcs.go line 654 at r3 (raw file):

Previously, msirek (Mark Sirek) wrote…

I benchmarked this as well and found opt.ColMap (util.FastIntMap) to be slower than a regular map and perform allocations, while the regular map does not.

func BenchmarkColumnIDMapMem(b *testing.B) {
	colID1 := []opt.ColumnID{1, 9, 100, 300, 2000, 10000}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		colMap := make(map[opt.ColumnID]opt.ColumnID)
		colMap[colID1[0]] = colID1[0]
		colMap[colID1[1]] = colID1[1]
		colMap[colID1[2]] = colID1[2]
		colMap[colID1[3]] = colID1[3]
		colMap[colID1[4]] = colID1[4]
		colMap[colID1[5]] = colID1[5]
	}
	b.StopTimer()
}

func BenchmarkFastIntMapMem(b *testing.B) {
	colID1 := []int{1, 9, 100, 300, 2000, 10000}
	b.ResetTimer()
	for i := 0; i < 1000000; i++ {
		colMap := util.FastIntMap{}
		colMap.Set(colID1[0], colID1[0])
		colMap.Set(colID1[1], colID1[1])
		colMap.Set(colID1[2], colID1[2])
		colMap.Set(colID1[3], colID1[3])
		colMap.Set(colID1[4], colID1[4])
		colMap.Set(colID1[5], colID1[5])
	}
	b.StopTimer()
}

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkFastIntMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkFastIntMapMem-32              1        1314439680 ns/op        1200006768 B/op  2000021 allocs/op
BenchmarkFastIntMapMem-32              1        1250450352 ns/op        1200000224 B/op  2000006 allocs/op
BenchmarkFastIntMapMem-32              1        1255882487 ns/op        1200000792 B/op  2000011 allocs/op
BenchmarkFastIntMapMem-32              1        1277546920 ns/op        1200000248 B/op  2000006 allocs/op
BenchmarkFastIntMapMem-32              1        1313803530 ns/op        1200000128 B/op  2000005 allocs/op
BenchmarkFastIntMapMem-32              1        1221278816 ns/op        1200000208 B/op  2000004 allocs/op
BenchmarkFastIntMapMem-32              1        1249782485 ns/op        1200000288 B/op  2000003 allocs/op
BenchmarkFastIntMapMem-32              1        1285993783 ns/op        1200000304 B/op  2000005 allocs/op
BenchmarkFastIntMapMem-32              1        1250126388 ns/op        1200000192 B/op  2000002 allocs/op
BenchmarkFastIntMapMem-32              1        1352475944 ns/op        1200000408 B/op  2000007 allocs/op

make bench PKG=./pkg/sql/opt/memo BENCHES=BenchmarkColumnIDMapMem 'TESTFLAGS=-count 10 -benchmem'
...
BenchmarkColumnIDMapMem-32      1000000000               0.1788 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1738 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1713 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1678 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1823 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1819 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1860 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1808 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1714 ns/op          0 B/op          0 allocs/op
BenchmarkColumnIDMapMem-32      1000000000               0.1775 ns/op          0 B/op          0 allocs/op

I think since the maps are only used local to this function, Go is smart enough to only use stack memory for them.

Interesting, thanks

---

pkg/sql/opt/xform/testdata/rules/join line 13039 at r3 (raw file):

Previously, msirek (Mark Sirek) wrote…

I'm not sure which rule to assert. GenerateLookupJoins is triggered whether or not any lookup joins are actually built.

For exploration rules, these assertions check whether a new expression was added, not whether the rule was matched.

---

pkg/sql/opt/xform/testdata/rules/join line 13066 at r3 (raw file):

Previously, msirek (Mark Sirek) wrote…

Same as above. ForeignKeyConstraintFilters is not conditionally triggered by any rule. Which rule were you thinking should be tested here?

See above

[msirek]

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

---

pkg/sql/opt/xform/testdata/rules/join line 13039 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

For exploration rules, these assertions check whether a new expression was added, not whether the rule was matched.

TIL
Thanks, added the expect assertion

Code quote:

# Lateral join which utilizes lookup join should derive predicates.

---

pkg/sql/opt/xform/testdata/rules/join line 13066 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

See above

Added the expect-not assertion

[rytaft]

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

[msirek]

TFTR!
bors r=rytaft

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)