sql: allow string concat with any non-array type #55611

rafiss · 2020-10-15T18:30:07Z

Many tools and ORMs that work with Postgres expect this behavior.

this is retrying the idea from #43190
fixes #41872
fixes #34404

Release note (sql change): the concatenation operator || is now usable
between strings and any other non-array type.

Release note (backward-incompatible change): Concatenation between a
non-null argument and a null argument is now typed as string
concatenation. (It previously used to be typed as array concatentation.)
This means that the result of NULL || 1 will now be NULL.
Previously, the result would be {1}. To preserve the old behavior, the NULL
argument can be casted to an explicit type.

cockroach-teamcity · 2020-10-15T18:30:14Z

This change is

RaduBerinde

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

pkg/sql/sem/tree/eval.go, line 479 at r1 (raw file):

				return nil, errors.New("neither LHS or RHS matched DString")
			},
			Volatility: VolatilityImmutable,

This is not correct. E.g. TIMESTAMPTZ to string is not an immutable cast. Look at the cast volatilities in tree/casts.go (validCasts). Perhaps looking up the volatility of the cast from the given type to string is the right way to initialize this.

yuzefovich · 2020-10-17T02:19:10Z

Before this PR, we fully supported concat binary operator in the vectorized engine, so it'd be good to either confirm that it is still the case or open up an issue to add the support (if the latter is the case, the new issue should be referenced in #49463).

rafiss · 2020-10-22T18:46:35Z

@yuzefovich I created #55841 (the new overloads do not work with colexec)

rafiss

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

pkg/sql/sem/tree/eval.go, line 479 at r1 (raw file):

Previously, RaduBerinde wrote…

This is not correct. E.g. TIMESTAMPTZ to string is not an immutable cast. Look at the cast volatilities in tree/casts.go (validCasts). Perhaps looking up the volatility of the cast from the given type to string is the right way to initialize this.

thanks for this tip. updated

otan · 2020-10-22T22:40:48Z

sooooo how hard is it to get this to work with anynonarray, to be more postgres-esque?

rafiss · 2020-10-23T21:16:57Z

I had a local branch with the anynonarray approach. but i didn't have an idea for dealing with the thing you got stuck with: #43143 (comment). but i do think i can dive into that problem more.

rafiss · 2020-10-23T21:54:31Z

hmm although to Radu's point above, we can't treat all the anynonarray overloads the same, since they have different volatility. unless we are allowed to be pessimistic and make them all VolatilityStable

rafiss · 2020-10-23T22:16:18Z

so i did get past the issue of #43143 (comment) just by adding explicit string-byte and byte-string concat overloads.

but after that, these cases failed:

    --- FAIL: TestEval/opt (0.07s)
        --- FAIL: TestEval/opt/concat (0.00s)
            eval_test.go:49:
                testdata/eval/concat:80: 1 || NULL
                expected:
                NULL

                found:
                ARRAY[1]
    --- FAIL: TestEval/no-opt (0.05s)
        --- FAIL: TestEval/no-opt/concat (0.00s)
            eval_test.go:49:
                testdata/eval/concat:80: 1 || NULL
                expected:
                NULL

                found:
                ARRAY[1]

with the anynonarray approach i haven't been able to think of a way to make the type checker pick string concat over array concat in the presence of nulls.

otan · 2020-10-26T14:53:28Z

unless we are allowed to be pessimistic and make them all VolatilityStable

this is what PostgreSQL does

otan

lgtm if you can't get anynonarray to work.

otan · 2020-10-26T14:59:45Z

pkg/sql/sem/tree/eval.go

@@ -417,9 +417,48 @@ func initArrayToArrayConcatenation() {
 	}
 }

+// initNonArrayToNonArrayConcatenation initializes string + nonarrayelement
+// and nonarrayelement + string concatenation.
+func initNonArrayToNonArrayConcatenation() {


can you add a test that checks <anyNonArrayElem> || NULL = anyNonArrayElem.String() for each non array elem type?

i may not be following, but in the operation you posted, the result should be NULL.

i added a test to pkg/sql/sem/tree/testdata/eval/concat. See https://github.com/rafiss/cockroach/blob/3705e3c7f37effaa644d45344b5c89a461877401/pkg/sql/sem/tree/testdata/eval/concat#L73

i can add more tests for the other non array types.

ah woops, i meant anyNonArrayElem || string, as a loop over all current types to make sure it doesn't get resolved unexpectedly.

it didn't seem very nice to loop over types and generate values for each type, so instead i just added more cases to concat eval test. it turned up an issue so i updated eval.go to use the FmtBareStrings flag

it didn't seem very nice to loop over types and generate values for each type

(i'm not opposed to this landing this as is, but curious)
does RandDatum not give you something nice?

i didn't know about RandDatum hehehe. thank you for the pointer!

will use that, and anyway looks like i need to fix tuple-string concat:

encountered internal error: no volatility for cast tuple{int, int}::tuple (1) assertion failure Wraps: (2) attached stack trace -- stack trace: | github.com/cockroachdb/cockroach/pkg/sql/opt/memo.BuildSharedProps | /go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/memo/logical_props_builder.go:1512

rafiss · 2020-10-30T19:47:43Z

I've had to update tree.ReType to get things working, so PTAL. I don't think that needs to handle recursive tuples, but let me know if it should.

otan · 2020-11-03T16:33:44Z

pkg/sql/sem/tree/normalize.go

+	}
+	// Casting to an "any" tuple is not allowed because there is no way to define
+	// the volatility in LookupCastVolatility.
+	if wantedType.Family() == types.TupleFamily &&


can we not always use stable as a volatility? i think that's what Postgres does. in these circumstances.

that sounds cool! will try.

Many tools and ORMs that work with Postgres expect this behavior. Release note (sql change): The concatenation operator || is now usable between strings and any other non-array type. Release note (backward-incompatible change): Concatenation between a non-null argument and a null argument is now typed as string concatenation. (It previously used to be typed as array concatentation.) This means that the result of `NULL || 1` will now be `NULL`. Previously, the result would be `{1}`. To preserve the old behavior, the NULL argument can be casted to an explicit type.

rafiss · 2020-11-06T16:37:25Z

thanks for reviewing :)

bors r=otan

craig · 2020-11-06T17:19:09Z

Build succeeded:

GitHub CI (Cockroach)

rafiss force-pushed the string-concat branch 2 times, most recently from a7b8529 to 018b753 Compare October 16, 2020 22:13

rafiss requested a review from a team as a code owner October 16, 2020 22:13

RaduBerinde reviewed Oct 17, 2020

View reviewed changes

rafiss force-pushed the string-concat branch 4 times, most recently from f79b917 to 316a312 Compare October 21, 2020 17:48

rafiss mentioned this pull request Oct 22, 2020

colexec: add support for concat binary op with string and another type #55841

Closed

rafiss force-pushed the string-concat branch 2 times, most recently from 0eaca7e to 3705e3c Compare October 22, 2020 17:53

rafiss requested review from otan and a team October 22, 2020 18:45

rafiss commented Oct 22, 2020

View reviewed changes

otan approved these changes Oct 26, 2020

View reviewed changes

rafiss force-pushed the string-concat branch 2 times, most recently from 09ffe25 to eae45c4 Compare October 30, 2020 19:45

rafiss requested a review from otan October 30, 2020 19:46

rafiss force-pushed the string-concat branch from eae45c4 to 0b6bbce Compare November 2, 2020 16:55

otan reviewed Nov 3, 2020

View reviewed changes

rafiss force-pushed the string-concat branch from 0b6bbce to 088cd29 Compare November 3, 2020 18:44

rafiss force-pushed the string-concat branch 3 times, most recently from d966b95 to 11dc8bb Compare November 6, 2020 00:19

rafiss force-pushed the string-concat branch from 11dc8bb to c8ac3ed Compare November 6, 2020 03:02

otan approved these changes Nov 6, 2020

View reviewed changes

craig bot merged commit fb15b37 into cockroachdb:master Nov 6, 2020

rafiss deleted the string-concat branch November 16, 2020 22:55

jseldess mentioned this pull request Dec 9, 2020

sql: allow string concat with any non-array type cockroachdb/docs#9076

Closed

lukaseder mentioned this pull request Mar 5, 2021

Add versioned dialects for COCKROACHDB 20, 21 jOOQ/jOOQ#11572

Closed

rafiss mentioned this pull request May 18, 2021

release-21.1: tree: make string concat do a normal cast #65331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: allow string concat with any non-array type #55611

sql: allow string concat with any non-array type #55611

rafiss commented Oct 15, 2020 •

edited

Loading

cockroach-teamcity commented Oct 15, 2020

RaduBerinde left a comment

yuzefovich commented Oct 17, 2020

rafiss commented Oct 22, 2020

rafiss left a comment

otan commented Oct 22, 2020

rafiss commented Oct 23, 2020

rafiss commented Oct 23, 2020

rafiss commented Oct 23, 2020

otan commented Oct 26, 2020

otan left a comment

otan Oct 26, 2020

rafiss Oct 26, 2020

otan Oct 26, 2020

rafiss Oct 27, 2020

otan Oct 28, 2020

rafiss Oct 28, 2020

rafiss commented Oct 30, 2020

otan Nov 3, 2020

rafiss Nov 3, 2020

rafiss commented Nov 6, 2020

craig bot commented Nov 6, 2020

sql: allow string concat with any non-array type #55611

sql: allow string concat with any non-array type #55611

Conversation

rafiss commented Oct 15, 2020 • edited Loading

cockroach-teamcity commented Oct 15, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

yuzefovich commented Oct 17, 2020

rafiss commented Oct 22, 2020

rafiss left a comment

Choose a reason for hiding this comment

otan commented Oct 22, 2020

rafiss commented Oct 23, 2020

rafiss commented Oct 23, 2020

rafiss commented Oct 23, 2020

otan commented Oct 26, 2020

otan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss commented Nov 6, 2020

craig bot commented Nov 6, 2020

rafiss commented Oct 15, 2020 •

edited

Loading