From 1c2d81301b6487a9b47d045788853a816f1b9d35 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 9 Oct 2023 14:53:32 -0600 Subject: [PATCH] sql: allow refcursor comparison with NULL values Most comparisons are disallowed for the REFCURSOR data type. For example, the following is not allowed in postgres: ``` SELECT 'foo'::REFCURSOR = 'bar'::REFCURSOR; ``` However, postgres does allow using `IS [NOT] DISTINCT FROM NULL` and `IS [NOT] NULL` with a refcursor-typed expression os the left operand. There are a few places where CRDB internally expects this comparison to be valid as well, so disallowing these comparisons has caused test failures. This patch removes the refcursor-specific check for invalid comparisons during type checking, and relies on execution-time checks as is done for other comparisons. This means that `IsNull` and `IsNotNull` expressions can now be executed. In addition, the `IS DISTINCT FROM NULL` comparison is now supported only for the case of type `REFCURSOR` as the left operand, and `UNKNOWN` (NULL) as the right operand. Fixes #112010 Fixes #112011 Release note: None --- docs/generated/sql/operators.md | 1 + .../logictest/testdata/logic_test/refcursor | 106 ++++++++++++++++-- pkg/sql/sem/tree/eval.go | 4 + pkg/sql/sem/tree/type_check.go | 20 ---- 4 files changed, 102 insertions(+), 29 deletions(-) diff --git a/docs/generated/sql/operators.md b/docs/generated/sql/operators.md index 21d27ad861c6..9f01f3508286 100644 --- a/docs/generated/sql/operators.md +++ b/docs/generated/sql/operators.md @@ -447,6 +447,7 @@ oid IS NOT DISTINCT FROM intbool oid IS NOT DISTINCT FROM oidbool pg_lsn IS NOT DISTINCT FROM pg_lsnbool +refcursor IS NOT DISTINCT FROM unknownbool string IS NOT DISTINCT FROM stringbool string[] IS NOT DISTINCT FROM string[]bool time IS NOT DISTINCT FROM timebool diff --git a/pkg/sql/logictest/testdata/logic_test/refcursor b/pkg/sql/logictest/testdata/logic_test/refcursor index 56fd0750201c..8558b231337c 100644 --- a/pkg/sql/logictest/testdata/logic_test/refcursor +++ b/pkg/sql/logictest/testdata/logic_test/refcursor @@ -440,29 +440,117 @@ SELECT 'foo'::REFCURSOR >= 'foo'::TEXT; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= NULL; -# TODO(drewk): Postgres allows this case. -statement error pgcode 42883 pq: unsupported comparison operator +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::TEXT; + +# Regression test for #112010 - REFCURSOR should allow IS NULL and +# IS NOT NULL comparisons. +subtest is_null + +query B SELECT 'foo'::REFCURSOR IS NULL; +---- +false -statement error pgcode 42883 pq: unsupported comparison operator +query B SELECT 'foo'::REFCURSOR IS NOT NULL; +---- +true + +query B +SELECT 'foo'::REFCURSOR IS DISTINCT FROM NULL; +---- +true + +query B +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM NULL; +---- +false + +# REFCURSOR columns are allowed. +query B rowsort +SELECT l IS NULL FROM implicit_types; +---- +true +true +true +true +true + +query B rowsort +SELECT l IS NOT NULL FROM implicit_types; +---- +false +false +false +false +false + +query B rowsort +SELECT l IS DISTINCT FROM NULL FROM implicit_types; +---- +false +false +false +false +false + +query B rowsort +SELECT l IS NOT DISTINCT FROM NULL FROM implicit_types; +---- +true +true +true +true +true +# Comparison with column is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM l FROM implicit_types; statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM l FROM implicit_types; +# Comparison with typed NULL is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS DISTINCT FROM NULL; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (NULL::REFCURSOR); statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (NULL::REFCURSOR); +# Comparison with CASE expression is not allowed. statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::TEXT; +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); statement error pgcode 22023 pq: unsupported comparison operator -SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM NULL; +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); + +# The CASE operator is folded into an untyped NULL, so it's allowed. +# TODO(drewk): Postgres doesn't allow this case. +query B +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); +---- +true + +query B +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); +---- +false + +# The CASE operator is folded into a typed NULL, so it's not allowed. +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); + +statement error pgcode 22023 pq: unsupported comparison operator +SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); subtest end diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 6748453f51ed..e9784271e94f 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -1775,6 +1775,10 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Void, types.Unknown, volatility.Leakproof), makeIsFn(types.Unknown, types.Void, volatility.Leakproof), + // REFCURSOR cannot be compared with itself, but as a special case, it can + // be compared with NULL using IS NOT DISTINCT FROM. + makeIsFn(types.RefCursor, types.Unknown, volatility.Leakproof), + // Tuple comparison. { LeftType: types.AnyTuple, diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index abd896057bdf..cbef0101ba1b 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -941,14 +941,6 @@ func (expr *ComparisonExpr) TypeCheck( return nil, err } - // REFCURSOR does not support comparisons. - leftType, rightType := leftTyped.ResolvedType(), rightTyped.ResolvedType() - if leftType.Family() == types.RefCursorFamily || rightType.Family() == types.RefCursorFamily { - return nil, pgerror.Newf(pgcode.UndefinedFunction, - "unsupported comparison operator: <%s> %s <%s>", leftType, cmpOp, rightType, - ) - } - if alwaysNull { return DNull, nil } @@ -1471,12 +1463,6 @@ func (expr *IsNullExpr) TypeCheck( if err != nil { return nil, err } - // REFCURSOR does not support comparisons. - if exprTyped.ResolvedType().Family() == types.RefCursorFamily { - return nil, pgerror.New(pgcode.UndefinedFunction, - "unsupported comparison operator: refcursor IS unknown", - ) - } expr.Expr = exprTyped expr.typ = types.Bool return expr, nil @@ -1490,12 +1476,6 @@ func (expr *IsNotNullExpr) TypeCheck( if err != nil { return nil, err } - // REFCURSOR does not support comparisons. - if exprTyped.ResolvedType().Family() == types.RefCursorFamily { - return nil, pgerror.New(pgcode.UndefinedFunction, - "unsupported comparison operator: refcursor IS NOT unknown", - ) - } expr.Expr = exprTyped expr.typ = types.Bool return expr, nil