opt: fix like escape processing for span constraints #73978

cucaroach · 2021-12-17T16:48:28Z

Previously no attempt was made to properly handle escape ('\') sequence
in like patterns being turned into constraints. Refactor code used to
process like at runtime to generate a regexp and use that to properly
handle index constraint generation.

Release note (sql change): Escape character processing was missing from
constraint span generation which resulted in incorrect results when
doing escaped like lookups.

cockroach-teamcity · 2021-12-17T16:48:35Z

This change is

mgartner

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @RaduBerinde)

pkg/sql/opt/idxconstraint/index_constraints.go, line 283 at r1 (raw file):

	case opt.LikeOp:
		if s, ok := tree.AsDString(datum); ok {
			// Normalize the like pattern to a RE

nit: spell out "regex" or "regular expression" and add a period.

pkg/sql/logictest/testdata/logic_test/select_index, line 257 at r1 (raw file):


statement ok
INSERT INTO str VALUES (1, 'A'), (4, 'AB'), (2, 'ABC'), (5, 'ABCD'), (3, 'ABCDEZ'), (9, 'ABD'), (10, '\CBA'), (11, 'A%'), (12, 'CAB.*')

nit: insert a value like 'CABD' so that the test below with v LIKE 'CAB.*' shows that only the row with 'CAB.*' is returned.

pkg/sql/opt/idxconstraint/testdata/strings, line 6 at r1 (raw file):

[/'ABC' - /'ABC']

# a backslash that isn't escaping anything is just removed from pattern

nit: treat comments as a sentence here and below with capital first letter and period at the end.

pkg/sql/opt/idxconstraint/testdata/strings, line 8 at r1 (raw file):

# a backslash that isn't escaping anything is just removed from pattern
index-constraints vars=(a string) index=a
a LIKE '\aABC%'

Can you add a test like a LIKE '\aABC_'? I think remaining filters are required in this case.

RaduBerinde

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

pkg/sql/opt/memo/constraint_builder.go, line 192 at r1 (raw file):

				c := cb.makeStringPrefixSpan(col, prefix)
				// If it ends in .* its tight.
				return c, strings.HasSuffix(pattern, ".*")

I don't understand this logic. Won't something like AB_C% end in .* but prefix is just AB ?

pkg/sql/opt/idxconstraint/testdata/strings, line 59 at r1 (raw file):

# backslash terminator is okay
index-constraints vars=(a string) index=a
a LIKE 'ABC\'

This should result in an error, no? It does currently:

[email protected]:26257/defaultdb> SELECT 'ABC' LIKE 'ABC\';
ERROR: LIKE regexp compilation failed: LIKE pattern must not end with escape character

If we convert to index constraints and execute, we might return an ABC row with no errors.

cucaroach

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

pkg/sql/opt/idxconstraint/index_constraints.go, line 283 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: spell out "regex" or "regular expression" and add a period.

👍

pkg/sql/opt/memo/constraint_builder.go, line 192 at r1 (raw file):

Previously, RaduBerinde wrote…

I don't understand this logic. Won't something like AB_C% end in .* but prefix is just AB ?

Nice catch, what I want is something stricter, I went with prefix+".*" == pattern but not sure it there's a better way that avoids string construction. Also added a test for this case.

pkg/sql/logictest/testdata/logic_test/select_index, line 257 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: insert a value like 'CABD' so that the test below with v LIKE 'CAB.*' shows that only the row with 'CAB.*' is returned.

👍

pkg/sql/opt/idxconstraint/testdata/strings, line 6 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: treat comments as a sentence here and below with capital first letter and period at the end.

👍

pkg/sql/opt/idxconstraint/testdata/strings, line 8 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you add a test like a LIKE '\aABC_'? I think remaining filters are required in this case.

👍

pkg/sql/opt/idxconstraint/testdata/strings, line 59 at r1 (raw file):

Previously, RaduBerinde wrote…

This should result in an error, no? It does currently:
[email protected]:26257/defaultdb> SELECT 'ABC' LIKE 'ABC\';
ERROR: LIKE regexp compilation failed: LIKE pattern must not end with escape character
If we convert to index constraints and execute, we might return an ABC row with no errors.

Nice catch, fixed and added a test for this.

RaduBerinde

Nice job!

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

pkg/sql/opt/memo/constraint_builder.go, line 192 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Nice catch, what I want is something stricter, I went with prefix+".*" == pattern but not sure it there's a better way that avoids string construction. Also added a test for this case.

strings.HasSuffix(pattern, ".*") && strings.TrimSuffix(pattern, ".*") == prefix

pkg/sql/opt/memo/constraint_builder.go, line 191 at r2 (raw file):

					}
					c := cb.makeStringPrefixSpan(col, prefix)
					// If pattern is simply prefix + .* its tight.

[nit] it's (or better yet "the span is tight"

pkg/sql/opt/idxconstraint/testdata/strings, line 71 at r2 (raw file):

[/'ABC_Z' - /'ABC_Z']

# Escape terminator is okay for index constraint purposes.

should say "Invalid pattern does not generate index constraints"

Fixes: cockroachdb#44123 Previously no attempt was made to properly handle escape ('\\') sequence in like patterns being turned into constraints. Refactor code used to process like at runtime to generate a regexp and use that to properly handle index constraint generation. Release note (sql change): Escape character processing was missing from constraint span generation which resulted in incorrect results when doing escaped like lookups.

cucaroach

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

pkg/sql/opt/memo/constraint_builder.go, line 191 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] it's (or better yet "the span is tight"

👍

mgartner

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

cucaroach · 2021-12-21T17:02:57Z

bors r+
Thanks for the reviews!

craig · 2021-12-21T18:42:29Z

Build succeeded:

GitHub CI (Cockroach)

rafiss · 2021-12-21T20:57:25Z

Thanks for fixing this! Is this eligible for a backport?

cucaroach · 2021-12-21T22:55:32Z

I would wager yes since its a correctness issue but I'm not sure, @jordanlewis?

jordanlewis · 2021-12-22T00:30:27Z

Yes, I agree we should backport.

rafiss · 2021-12-23T21:36:38Z

blathers backport 21.1 21.2

cucaroach force-pushed the like-constraint-fix branch from d2d2f14 to 5586923 Compare December 18, 2021 11:54

cucaroach requested a review from RaduBerinde December 18, 2021 11:56

cucaroach marked this pull request as ready for review December 18, 2021 11:59

cucaroach requested a review from a team as a code owner December 18, 2021 11:59

cucaroach force-pushed the like-constraint-fix branch from 5586923 to ae6d31b Compare December 20, 2021 22:06

mgartner reviewed Dec 20, 2021

View reviewed changes

RaduBerinde reviewed Dec 20, 2021

View reviewed changes

cucaroach force-pushed the like-constraint-fix branch 2 times, most recently from 6313ceb to 1f63d36 Compare December 20, 2021 23:30

cucaroach commented Dec 20, 2021

View reviewed changes

cucaroach force-pushed the like-constraint-fix branch from 1f63d36 to 306ec93 Compare December 20, 2021 23:43

RaduBerinde approved these changes Dec 21, 2021

View reviewed changes

cucaroach force-pushed the like-constraint-fix branch 3 times, most recently from 572da7f to 6b8bbfc Compare December 21, 2021 12:36

cucaroach force-pushed the like-constraint-fix branch from 6b8bbfc to d25385b Compare December 21, 2021 15:51

cucaroach commented Dec 21, 2021

View reviewed changes

mgartner approved these changes Dec 21, 2021

View reviewed changes

craig bot merged commit 779f08b into cockroachdb:master Dec 21, 2021

cockroach-teamcity mentioned this pull request Dec 21, 2021

opt: fix like escape processing for span constraints cockroachdb/docs#12641

Open

rafiss added backport-21.1.x labels Dec 23, 2021

This was referenced Dec 23, 2021

release-21.1: opt: fix like escape processing for span constraints #74258

Merged

release-21.2: opt: fix like escape processing for span constraints #74259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: fix like escape processing for span constraints #73978

opt: fix like escape processing for span constraints #73978

cucaroach commented Dec 17, 2021 •

edited

Loading

cockroach-teamcity commented Dec 17, 2021

mgartner left a comment

RaduBerinde left a comment

cucaroach left a comment

RaduBerinde left a comment

cucaroach left a comment

mgartner left a comment

cucaroach commented Dec 21, 2021

craig bot commented Dec 21, 2021

rafiss commented Dec 21, 2021

cucaroach commented Dec 21, 2021

jordanlewis commented Dec 22, 2021

rafiss commented Dec 23, 2021

opt: fix like escape processing for span constraints #73978

opt: fix like escape processing for span constraints #73978

Conversation

cucaroach commented Dec 17, 2021 • edited Loading

cockroach-teamcity commented Dec 17, 2021

mgartner left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

cucaroach commented Dec 21, 2021

craig bot commented Dec 21, 2021

rafiss commented Dec 21, 2021

cucaroach commented Dec 21, 2021

jordanlewis commented Dec 22, 2021

rafiss commented Dec 23, 2021

cucaroach commented Dec 17, 2021 •

edited

Loading