colexec: fix IS NOT NULL filter for tuples with NULLs

[yuzefovich]

This commit fixes isTupleNullSelOp (which handles IS NOT NULL filter expression). Previously, we incorrectly short-circuited the filter evaluation whenever the tuple is non-NULL - all such tuples were passed by the filter even though only tuples that don't have any NULL elements should pass. This is now fixed by bringing the logic inline with how row-by-row engine does it as well as the isTupleNullProjOp does it.

This commit additionally extends an existing vectorized eval test (which currently runs all projections) to also run filtering (i.e. "selection" operators) on expressions of the boolean type. This would have caught the bug.

Fixes: #126769.

Release note (bug fix): Previously, CockroachDB could incorrectly evaluate IS NOT NULL filter if it was applied to non-NULL tuples that had NULL elements (like (1, NULL) or (NULL, NULL)). The bug is present since 20.2 version and is now fixed.

[cockroach-teamcity]

This change is 

[DrewKimball]

Just some nits.

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

---

pkg/sql/colexec/is_null_ops_tmpl.go line 252 at r1 (raw file):

	} else {
		selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
	}

[nit] I think it'd be more clear to do this:

tupleIsNull := nulls.NullAt(i)
if !tupleIsNull {
  selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
}
if tupleIsNull != o.negate {
  ...
}

---

pkg/sql/logictest/testdata/logic_test/tuple line 1324 at r1 (raw file):

SELECT * FROM (SELECT (1, 2) AS a) AS b WHERE a IS NOT NULL;
----
(1,2)

Do we also handle an empty tuple correctly?

postgres=# select row() is null;
 ?column?
----------
 t
(1 row)

postgres=# select row() is not null;
 ?column?
----------
 t
(1 row)

[yuzefovich]

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

---

pkg/sql/colexec/is_null_ops_tmpl.go line 252 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] I think it'd be more clear to do this:

tupleIsNull := nulls.NullAt(i)
if !tupleIsNull {
  selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
}
if tupleIsNull != o.negate {
  ...
}

Hm, I don't quite follow this logic.

What we want is the following:

• we have IS NULL expr, so negate == false
  • we want to select the tuple if it is NULL datum or a tuple only with NULL elements
• we have IS NOT NULL expr, so negate == true
  • we want to select the tuple if it is non-NULL datum and all its elements are non-NULL.

So this snippet could look like

	tupleIsNull := nulls.NullAt(i)
	var selectTuple bool
	if !tupleIsNull {
		selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
	}
	if (!o.negate && (tupleIsNull || selectTuple)) || (o.negate && selectTuple) {
		sel[idx] = i
		idx++
	}

But I find the current code a bit cleaner (and it matches what we have in _COMPUTE_IS_NULL for the projection). Thoughts?

---

pkg/sql/logictest/testdata/logic_test/tuple line 1324 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Do we also handle an empty tuple correctly?

postgres=# select row() is null;
 ?column?
----------
 t
(1 row)

postgres=# select row() is not null;
 ?column?
----------
 t
(1 row)

Yeah, we do. I added these as explicit tests (plus we also have them in eval/testdata/eval/is).

[DrewKimball]

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

---

pkg/sql/colexec/is_null_ops_tmpl.go line 252 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Hm, I don't quite follow this logic.

What we want is the following:

• we have IS NULL expr, so negate == false
  • we want to select the tuple if it is NULL datum or a tuple only with NULL elements
• we have IS NOT NULL expr, so negate == true
  • we want to select the tuple if it is non-NULL datum and all its elements are non-NULL.

So this snippet could look like

	tupleIsNull := nulls.NullAt(i)
	var selectTuple bool
	if !tupleIsNull {
		selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
	}
	if (!o.negate && (tupleIsNull || selectTuple)) || (o.negate && selectTuple) {
		sel[idx] = i
		idx++
	}

But I find the current code a bit cleaner (and it matches what we have in _COMPUTE_IS_NULL for the projection). Thoughts?

Oh, I missed that isTupleNull incorporates o.negate - I thought it just checked if the elements were null. The current logic looks fine, then.

[yuzefovich]

TFTR!

bors r+

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

---

pkg/sql/colexec/is_null_ops_tmpl.go line 252 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Oh, I missed that isTupleNull incorporates o.negate - I thought it just checked if the elements were null. The current logic looks fine, then.

np!

[craig]

Build succeeded:

• linux_amd64_fips_build
• local_roachtest_fips
• lint
• windows_build
• examples_orms
• linux_amd64_build
• docker_image_amd64
• unit_tests
• local_roachtest
• macos_arm64_build
• check_generated_code
• linux_arm64_build
• macos_amd64_build
• acceptance

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 42435a0 to blathers/backport-release-23.1-126901: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[mgartner]

blathers backport 24.2

[blathers-crl]

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

---

Issue #126769: branch-release-24.2.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}