opt: prevent cycle in memo created by SplitDisjunction

[mgartner]

opt: prevent cycle in memo created by SplitDisjunction

Previously, the SplitDisjunction rule would reuse table and column IDs
from the rule's input expressions in the left side of the generated
UnionAll expression. This was incorrectly believed to be safe. The
addition of partial indexes has provided an example that highlights the
flaw: this can create cycles in the memo.

Consider the table and query below:

CREATE TABLE t (
  k INT PRIMARY KEY,
  a INT,
  b INT,
  c INT,
  INDEX tab_c_idx (c) WHERE a > 1 OR b > 1
)

SELECT * FROM t WHERE a > 1 OR b > 1

This leads to a cycle in the memo because tab_c_idx can be scanned to
retrieve rows where a > 1 and where a > 1 OR b > 1. Notice the cycle
in the memo below: G2 -> G6 -> G10 -> G2. G2 includes both an
unconstrained partial index scan a DistinctOn created by
SplitDisjunction. The child of the DistinctOn is a UnionAll (G7)
with a left child of G10. G10 has two expressions, one of which is a
Select with an input of G2.

memo
 ├── G1: (scalar-group-by G2 G3)
 ├── G2: (select G4 G5) (scan t@tab_c_idx) (distinct-on G6 G7)
 ├── G3: (aggregations G8)
 ├── G4: (scan t)
 ├── G5: (filters G9)
 ├── G6: (union-all G10 G11)
 ├── G7: (aggregations G12)
 ├── G8: (count-rows)
 ├── G9: (or G13 G14)
 ├── G10: (select G4 G15) (select G2 G15)
 ├── G11: (select G16 G17)
 ├── G12: (const-agg G18)
 ├── G13: (eq G19 G20)
 ├── G14: (eq G18 G20)
 ├── G15: (filters G13)
 ├── G16: (scan t)
 ├── G17: (filters G21)
 ├── G18: (variable b)
 ├── G19: (variable a)
 ├── G20: (const 1)
 ├── G21: (eq G22 G20)
 └── G22: (variable b)

In order to prevent this cycle, SplitDisjunction now creates new table
and column IDs for both its left and right inputs. I've been unable to
find an example where table and column ID reuse in
SplitDisjuctionAddKey causes problems, but I've updated that rule to
err on the side of caution.

Fixes #58390

Release justification: This is a critical fix for a bug that causes
errors querying tables with disjunctive filters that are the same or
similar to the predicate of one of the table's partial indexes.

Release note (bug fix): A bug has been fixed which caused errors when
querying a table with a disjunctive filter (an OR expression) that is
the same or similar to the predicate of one of the table's partial
indexes.

[cockroach-teamcity]

This change is 

[rytaft]

nice find!

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, and @RaduBerinde)

---

pkg/sql/opt/xform/select_funcs.go, line 1731 at r1 (raw file):

// ScanPrivateTable returns the table ID of the scan private.
func (c *CustomFuncs) ScanPrivateTable(sp *memo.ScanPrivate) opt.TableID {

[nit] I'd call this something like TableIDFromScanPrivate or GetTableIDFromScanPrivate.

---

pkg/sql/opt/xform/testdata/rules/select, line 4861 at r1 (raw file):

      ├── union-all
      │    ├── columns: k:1!null u:2 v:3
      │    ├── left columns: k:1!null u:2 v:3

Interesting that we're losing this info about not-null columns. Not necessary for this PR, but maybe worth trying to add it back at some point?

---

pkg/sql/opt/xform/testdata/rules/select, line 5804 at r1 (raw file):

      ├── columns: k:1!null u:2!null v:3
      ├── grouping columns: k:1!null
      ├── internal-ordering: +1 opt(2)

why did we lose the ordering info here? (Maybe open an issue for this too)

[mgartner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @RaduBerinde, and @rytaft)

---

pkg/sql/opt/xform/select_funcs.go, line 1731 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] I'd call this something like TableIDFromScanPrivate or GetTableIDFromScanPrivate.

Done.

---

pkg/sql/opt/xform/testdata/rules/select, line 4861 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Interesting that we're losing this info about not-null columns. Not necessary for this PR, but maybe worth trying to add it back at some point?

Only set operators' output columns are added to the rel prop NotNullCols. You can see the that right column k:6 (now k:11) was not marked as null. I'm not sure what the implications are if NotNullCols contains columns not in the output of the set operation.

---

pkg/sql/opt/xform/testdata/rules/select, line 5804 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why did we lose the ordering info here? (Maybe open an issue for this too)

I'm guessing that ordering is no longer propagating due to the scans being duplicated with new column IDs. We'd need some way to translate the ordering across the projection. I've created an issue: #58435

[rytaft]

Reviewed 6 of 6 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)

---

pkg/sql/opt/xform/testdata/rules/select, line 4861 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Only set operators' output columns are added to the rel prop NotNullCols. You can see the that right column k:6 (now k:11) was not marked as null. I'm not sure what the implications are if NotNullCols contains columns not in the output of the set operation.

Ah I see -- makes sense. Thanks!

---

pkg/sql/opt/xform/testdata/rules/select, line 5804 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm guessing that ordering is no longer propagating due to the scans being duplicated with new column IDs. We'd need some way to translate the ordering across the projection. I've created an issue: #58435

Thanks!

[mgartner]

TFTR!

bors r=rytaft

[craig]

Build failed:

• GitHub CI (Cockroach)

[mgartner]

bors r=rytaft

[craig]

Build succeeded:

• GitHub CI (Cockroach)