sql: add support for DELETE FROM ... USING

[faizaanmadhani]

See commit messages for details.

Resolves: #40963

[cockroach-teamcity]

This change is 

[faizaanmadhani]

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

---

pkg/sql/opt/optbuilder/mutation_builder.go line 446 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm alarmed that prior to this commit we called setFetchColIDs with some potential non-fetch columns... I guess that didn't cause any problems. Regardless, moving setFetchColIDs up looks more correct to me. 👍

Done.

---

pkg/sql/opt/optbuilder/testdata/delete line 490 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Some tests to add:

• test the validateJoinTableNames path by putting the same table in the USING clause multiple times

Done. This test is added below (second last test in file).

[mgartner]

Reviewed 15 of 15 files at r4, 17 of 17 files at r9, 5 of 5 files at r10, 10 of 10 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @faizaanmadhani and @michae2)

---

-- commits line 34 at r11:
nit: cockroachdb => CockroachDB

---

pkg/sql/delete.go line 165 at r11 (raw file):

	var pm row.PartialIndexUpdateHelper
	if n := len(d.run.td.tableDesc().PartialIndexes()); n > 0 {
		offset := d.run.partialIndexDelValsOffset + d.run.numPassthrough

To keep d.run.partialIndexDelValsOffset true to its name, I think we should update it's value here instead:

cockroach/pkg/sql/opt_exec_factory.go

Line 1699 in 3f87f2f

partialIndexDelValsOffset: len(rd.FetchCols),

And the comment for it here shuld be updated too:

cockroach/pkg/sql/delete.go

Lines 52 to 54 in 961e66f

	// partialIndexDelValsOffset is the offset of partial index delete
	// indicators in the source values. It is equal to the number of fetched
	// columns.

---

pkg/sql/opt/optbuilder/mutation_builder.go line 487 at r10 (raw file):

		}

		if !pkCols.Empty() {

nit: A table must have PK columns, so this check is unnecessary.

---

pkg/sql/logictest/testdata/logic_test/delete line 338 at r11 (raw file):

5

# Test that USING works

nit: add period to the end of this comment and the ones below

---

pkg/sql/logictest/testdata/logic_test/delete line 345 at r11 (raw file):

    b STRING,
    c INT
);

nit: for single-statements in statement ok, there is no need for the ; at the end. I don't think we are terribly consistent, so it's up to you, but I think for the most part we leave it off.

---

pkg/sql/logictest/testdata/logic_test/delete line 366 at r11 (raw file):

);

statement count 4

nit: statement ok is fine here and below IMO - you're not testing INSERT so you don't need additional assertions

---

pkg/sql/logictest/testdata/logic_test/delete line 376 at r11 (raw file):

query ITI rowsort
SELECT * FROM u_a;

nit: you can remove this SELECT test

---

pkg/sql/logictest/testdata/logic_test/delete line 383 at r11 (raw file):

4  d  40

#Test a join with a filter

nit: space after #

---

pkg/sql/logictest/testdata/logic_test/delete line 462 at r11 (raw file):

# Test aliased table names, ORDER BY and LIMIT where ORDER BY references the target
# table.
query ITIIT rowsort

nit: remove rowsort since you have the ORDER BY in the query

---

pkg/sql/logictest/testdata/logic_test/delete line 483 at r11 (raw file):

query ITI rowsort
SELECT * FROM u_a;

nit: this looks redundant

---

pkg/sql/logictest/testdata/logic_test/delete line 528 at r11 (raw file):

CREATE TABLE pindex (
  a DECIMAL (10, 2),
  CHECK (a > 0)

There's no partial index here, and no need for the check constraint.

---

pkg/sql/opt/exec/explain/testdata/gists line 746 at r11 (raw file):

opt
DELETE FROM foo WHERE a = 1
----

nit: this looks like left-over from testing that you can remove

---

pkg/sql/opt/optbuilder/testdata/delete line 754 at r2 (raw file):

Previously, faizaanmadhani (Faizaan Madhani) wrote…

Done. Test case maintained but comment changed and test to check partial indexes added below.

CHECK constraints aren't checked during deletes, so you can remove the CHECK, add the parital index directly into this CREATE TABLE, and remove the test directly below this one. I'd update the table name to match too.

---

pkg/sql/opt/optbuilder/testdata/delete line 800 at r10 (raw file):

DELETE FROM abcde AS foo USING xyz AS bar WHERE bar.y > 0 ORDER BY foo.a DESC LIMIT 5;
----
error (42P10): SELECT DISTINCT ON expressions must match initial ORDER BY expressions

This should succeed, shouldn't it?

---

pkg/sql/opt/optbuilder/testdata/delete line 1106 at r10 (raw file):

      │              └── column2:14
      └── projections
           └── h:9 > 3 [as=partial_index_del1:15]

It'd be good to add a test where the target table has a compound primary key (more than 1 PK column, e.g. CREATE TABLE (h INT, i INT, j INT, PRIMARY KEY (h, i))) to ensure that the distinct-on groups by all the PK columns.

[faizaanmadhani]

pkg/sql/opt/optbuilder/testdata/delete line 800 at r10 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

This should succeed, shouldn't it?

(I think) It should. We have a similar logictest that also uses the target table in the ORDER BY that passes, so this is confusing.

After some digging, I figured out that this only happens if we're ordering on the non-primary key column of the target table while having a USING. Existing functionality does allow to do an ORDER BY with a non-primary key column in DELETE statements without USING clauses.

Similarly, if we check UPDATE ... FROM doing an ORDER BY with a column of a table that is not a primary key, this works so this test should pass.

[faizaanmadhani]

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

---

pkg/sql/delete.go line 165 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

To keep d.run.partialIndexDelValsOffset true to its name, I think we should update it's value here instead:

cockroach/pkg/sql/opt_exec_factory.go

Line 1699 in 3f87f2f

partialIndexDelValsOffset: len(rd.FetchCols),

And the comment for it here shuld be updated too:

cockroach/pkg/sql/delete.go

Lines 52 to 54 in 961e66f

	// partialIndexDelValsOffset is the offset of partial index delete
	// indicators in the source values. It is equal to the number of fetched
	// columns.

Done.

---

pkg/sql/logictest/testdata/logic_test/delete line 528 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

There's no partial index here, and no need for the check constraint.

Yup. I changed it to a partial index and modified the test for it

---

pkg/sql/opt/optbuilder/testdata/delete line 754 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

CHECK constraints aren't checked during deletes, so you can remove the CHECK, add the parital index directly into this CREATE TABLE, and remove the test directly below this one. I'd update the table name to match too.

Done. Removed the check constraint test below and added a partial index test using table name pindex

---

pkg/sql/opt/optbuilder/testdata/delete line 800 at r10 (raw file):

Previously, faizaanmadhani (Faizaan Madhani) wrote…

(I think) It should. We have a similar logictest that also uses the target table in the ORDER BY that passes, so this is confusing.

After some digging, I figured out that this only happens if we're ordering on the non-primary key column of the target table while having a USING. Existing functionality does allow to do an ORDER BY with a non-primary key column in DELETE statements without USING clauses.

Similarly, if we check UPDATE ... FROM doing an ORDER BY with a column of a table that is not a primary key, this works so this test should pass.

Resolved offline. While this should ideally succeed this failure happens when we do ORDER BY on non-primary key columns in a USING in tables when primary keys are present . This is related to this issue that's also present in UPDATE ... FROM: #89817

[mgartner]

Great work! just left one test suggestion.

Reviewed 12 of 12 files at r12, 9 of 10 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @faizaanmadhani, @michae2, and @rytaft)

---

pkg/sql/logictest/testdata/logic_test/delete line 527 at r14 (raw file):

3.00
4.00
8.00

nit: add a test for SELECT a FROM pindex@pindex_a_idx WHERE a > 3 to verify that scanning the partial index yields the same results as the full table scan here.

[faizaanmadhani]

TFTR!!!! 🎉

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)