Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree: permit NULL sequences to be typed as desired #36673

Merged
merged 1 commit into from Apr 30, 2019

Conversation

Projects
None yet
4 participants
@jordanlewis
Copy link
Member

commented Apr 9, 2019

Fixes #36607.

Previously, expressions that contained a sequence of expressions
(anything that uses TypeCheckSameTypedExprs) would fail to coerce
those expressions to a desired type if all of those expressions were
statically typed as NULL. Concretely, this led to expressions like

ARRAY[NULL, NULL]:::int[]

to not be type-checkable.

Also, make sure that arrays that contain only null are serialized
properly, by ensuring that they get a type annotation to prevent
ambiguity.

Release note (bug fix): permit arrays and other sequences to be
type-checkable in certain contexts when all of their elements are null.

@jordanlewis jordanlewis requested a review from mjibson Apr 9, 2019

@jordanlewis jordanlewis requested a review from cockroachdb/sql-rest-prs as a code owner Apr 9, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

This change is Reviewable

@RaduBerinde
Copy link
Member

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @mjibson)


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

	// The type of the array is ambiguous if it is empty or all-null; when
	// serializing we need to annotate it with the type.
	return len(d.Array) == 0 || !d.HasNonNulls

first part of the condition isn't needed


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

		t := types.UnwrapType(desired)
		if arrTyp, ok := t.(types.TArray); ok {
			d.ParamTyp = arrTyp.Typ

Is it cool to modify the ParamTyp in place? I think this means that if you call TypeCheck again, perhaps with a different desired (e.g. for an AST that was stored in a prepared statement, and there was a schema change) you might get unexpected results.

@jordanlewis jordanlewis force-pushed the jordanlewis:null-array-type branch from c2ba272 to cf970e6 Apr 9, 2019

@jordanlewis
Copy link
Member Author

left a comment

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


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

Previously, RaduBerinde wrote…

Is it cool to modify the ParamTyp in place? I think this means that if you call TypeCheck again, perhaps with a different desired (e.g. for an AST that was stored in a prepared statement, and there was a schema change) you might get unexpected results.

Good point. Fixed.

@mjibson

mjibson approved these changes Apr 9, 2019

@RaduBerinde
Copy link
Member

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @mjibson)


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

		{`ARRAY[1.5, 2.5, 3.5]`, `ARRAY[1.5:::DECIMAL, 2.5:::DECIMAL, 3.5:::DECIMAL]`},
		{`ARRAY[NULL]`, `ARRAY[NULL]`},
		{`ARRAY[NULL]:::int[]`, `ARRAY[NULL]`},

This is the output of Serialize, shouldn't it have a :::int[] ?

@jordanlewis
Copy link
Member Author

left a comment

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


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

Previously, RaduBerinde wrote…

This is the output of Serialize, shouldn't it have a :::int[] ?

Yeah.. it should. Hmm... also some tests are failing, there's some more work to do here.

@jordanlewis jordanlewis force-pushed the jordanlewis:null-array-type branch from cf970e6 to 809bd3b Apr 9, 2019

@jordanlewis
Copy link
Member Author

left a comment

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


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

Previously, jordanlewis (Jordan Lewis) wrote…

Yeah.. it should. Hmm... also some tests are failing, there's some more work to do here.

Actually, no this is correct, since it's a type assertion and not a cast. This is working as expected after the latest changes. The problem was that types.Any was permitted to sneak into some places that it shouldn't.

@mjibson

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

What's the status of this PR?

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

It has a bug and I haven't had time to fix it. I'm still planning to. (Feel free to take it over if you have the bandwidth and/or inclination - the bug is reproducible by running make testbaselogic)

@jordanlewis jordanlewis force-pushed the jordanlewis:null-array-type branch from 809bd3b to bef18c0 Apr 27, 2019

@jordanlewis jordanlewis requested review from cockroachdb/sql-async-prs as code owners Apr 27, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

Alright, should be RFAL again.

@jordanlewis jordanlewis force-pushed the jordanlewis:null-array-type branch 2 times, most recently from 7f3b9a4 to ce23e6f Apr 29, 2019

@jordanlewis jordanlewis requested a review from cockroachdb/distsql-prs as a code owner Apr 29, 2019

tree: permit NULL sequences to be typed as desired
Previously, expressions that contained a sequence of expressions
(anything that uses `TypeCheckSameTypedExprs`) would fail to coerce
those expressions to a desired type if all of those expressions were
statically typed as `NULL`. Concretely, this led to expressions like

```
ARRAY[NULL, NULL]:::int[]
```

to not be type-checkable.

Also, make sure that arrays that contain only null are serialized
properly, by ensuring that they get a type annotation to prevent
ambiguity.

Release note (bug fix): permit arrays and other sequences to be
type-checkable in certain contexts when all of their elements are null.
@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Apr 30, 2019

Merge #36673
36673: tree: permit NULL sequences to be typed as desired r=jordanlewis a=jordanlewis

Fixes #36607.

Previously, expressions that contained a sequence of expressions
(anything that uses `TypeCheckSameTypedExprs`) would fail to coerce
those expressions to a desired type if all of those expressions were
statically typed as `NULL`. Concretely, this led to expressions like

```
ARRAY[NULL, NULL]:::int[]
```

to not be type-checkable.

Also, make sure that arrays that contain only null are serialized
properly, by ensuring that they get a type annotation to prevent
ambiguity.

Release note (bug fix): permit arrays and other sequences to be
type-checkable in certain contexts when all of their elements are null.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Apr 30, 2019

Build succeeded

@craig craig bot merged commit ce23e6f into cockroachdb:master Apr 30, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@jordanlewis jordanlewis deleted the jordanlewis:null-array-type branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.