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

sql: fix TypeCheck for NULLIF #44718

Merged
merged 1 commit into from
Feb 4, 2020
Merged

sql: fix TypeCheck for NULLIF #44718

merged 1 commit into from
Feb 4, 2020

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Feb 4, 2020

Prior to this commit, TypeCheck for NULLIF would set the type of the
expression NULLIF(NULL, 0) to be int. However, the correct type is
actually unknown, since the type of NULL is unknown. This commit
fixes the error by using the type of the first expression as the type
of NULLIF.

Fixes #44632

Release note (bug fix): Fixed an internal error that could occur when
NULLIF was called with one null argument.

Prior to this commit, TypeCheck for NULLIF would set the type of the
expression NULLIF(NULL, 0) to be int. However, the correct type is
actually unknown, since the type of NULL is unknown. This commit
fixes the error by using the type of the first expression as the type
of NULLIF.

Fixes cockroachdb#44632

Release note (bug fix): Fixed an internal error that could occur when
NULLIF was called with one null argument.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)

Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball)

craig bot pushed a commit that referenced this pull request Feb 4, 2020
44718: sql: fix TypeCheck for NULLIF r=rytaft a=rytaft

Prior to this commit, `TypeCheck` for `NULLIF` would set the type of the
expression `NULLIF(NULL, 0)` to be `int`. However, the correct type is
actually `unknown`, since the type of `NULL` is `unknown`. This commit
fixes the error by using the type of the first expression as the type
of `NULLIF`.

Fixes #44632

Release note (bug fix): Fixed an internal error that could occur when
NULLIF was called with one null argument.

Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Feb 4, 2020

Build succeeded

@craig craig bot merged commit 4c86310 into cockroachdb:master Feb 4, 2020
@RaduBerinde
Copy link
Member

I realized I am actually a little confused here. NULLIF(NULL, 0) implies that NULL gets compared for equality to 0 so we can correctly imply that the type of NULL is integer.. I tried this in postgres:

radu=> SELECT pg_typeof(NULLIF(NULL, 0));
 pg_typeof 
-----------
 integer
(1 row)

@rytaft
Copy link
Collaborator Author

rytaft commented Feb 5, 2020

Hmm interesting. I'll hold off on the backports and look into this more. Thanks for checking on this!

@rytaft rytaft deleted the nullif2 branch February 5, 2020 17:46
@andy-kimball
Copy link
Contributor

Good catch. My mind just jumped to the assumption that we'd type according to the first argument, without type inference. This means that the bug is with this code in scalar.go:

	case *tree.NullIfExpr:
		input := b.buildScalar(t.Expr1.(tree.TypedExpr), inScope, nil, nil, colRefs)
		cond := b.buildScalar(t.Expr2.(tree.TypedExpr), inScope, nil, nil, colRefs)
		whens := memo.ScalarListExpr{b.factory.ConstructWhen(cond, memo.NullSingleton)}
		out = b.factory.ConstructCase(input, whens, input)

The resulting expression is not typed as null:::int like the t expression is. If we had an assert like this at the end of buildScalar, it would fail:

	if out.DataType().Identical(scalar.ResolvedType()) {
		panic("mismatched data types")
	}

The fix for the issue is to use something like tree.ReType on the input argument, similar to what the *tree.BinaryExpr case does. Also, take a look at other scalar ops in that switch; there could be other similar problems with NULL values, if both these 2 cases have problems. It's also evident we could use more tests of NULL argument expressions in the optbuilder tests.

@rytaft
Copy link
Collaborator Author

rytaft commented Feb 24, 2020

Thanks for identifying the fix, @andy-kimball (and finding the issue, @RaduBerinde!).

I've made the suggested change in #45354. Also added a few more test cases for both if and nullif.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal error for arithmetic operators and NULLIF
4 participants