Repository: geldata/gel
Pull request: Filter NULL arguments out when calling non-strict functions/operators
We have two related problems with a related solution:
 - Our implementation of the AND operator, which carefully
   ensures that we return NULL if either input is NULL,
   is a huge performance problem that prevents the use of indexes
 - User defined functions with implementations that aren't strict in
   their arguments can end up producing values when they shouldn't.

To fix this, we add NOT NULL filters when calling functions/operators
with potentially NULL values. To avoid doing this *too* much (and to
make it work more often in singleton mode), we add a notion of
function/operators having a strict implementation, which allows
the check to be skipped. Currently all built-in functions and operators
except for AND and OR are marked as being strict.

Fixes some #3185 related issues.
Fixes #3195.

Problem statement:
Functions don't always have correct strictness behavior with empty sets
Consider, in our `issues` test repo:
```
create function foo(x: str, y: int64) -> str USING (x);

SELECT BooleanTest { name, val, x := foo(.name, .val) };
```

Here, we'll incorrectly output `.name`, even when `.val` is empty.

This only happens downstream of something actually being represented as `NULL` in SQL, like when it comes from a table; `SELECT foo("test", (SELECT 1 FILTER false))` behaves correctly

Hints from issue discussion:

Commit message: Filter NULL arguments out when calling non-strict functions/operators

We have two related problems with a related solution:
 - Our implementation of the AND operator, which carefully
   ensures that we return NULL if either input is NULL,
   is a huge performance problem that prevents the use of indexes
 - User defined functions with implementations that aren't strict in
   their arguments can end up producing values when they shouldn't.

To fix this, we add NOT NULL filters when calling functions/operators
with potentially NULL values. To avoid doing this *too* much (and to
make it work more often in singleton mode), we add a notion of
function/operators having a strict implementation, which allows
the check to be skipped. Currently all built-in functions and operators
except for AND and OR are marked as being strict.

Fixes some #3185 related issues.
Fixes #3195.

Patch to review:
diff --git a/edb/buildmeta.py b/edb/buildmeta.py
index a79ab533eb6..2401aae598f 100644
--- a/edb/buildmeta.py
+++ b/edb/buildmeta.py
@@ -38,7 +38,7 @@
 
 
 # Increment this whenever the database layout or stdlib changes.
-EDGEDB_CATALOG_VERSION = 2021_11_10_00_00
+EDGEDB_CATALOG_VERSION = 2021_11_17_00_00
 
 
 class MetadataError(Exception):
diff --git a/edb/edgeql/compiler/func.py b/edb/edgeql/compiler/func.py
index a789505e66a..1751f1c099e 100644
--- a/edb/edgeql/compiler/func.py
+++ b/edb/edgeql/compiler/func.py
@@ -245,6 +245,11 @@ def compile_FunctionCall(
     else:
         tuple_path_ids = []
 
+    impl_is_strict = (
+        func.get_impl_is_strict(env.schema)
+        and func.get_language(env.schema) != qlast.Language.EdgeQL
+    )
+
     fcall = irast.FunctionCall(
         args=final_args,
         func_shortname=func_name,
@@ -269,6 +274,7 @@ def compile_FunctionCall(
         variadic_param_type=variadic_param_type,
         func_initial_value=func_initial_value,
         tuple_path_ids=tuple_path_ids,
+        impl_is_strict=impl_is_strict,
     )
 
     ir_set = setgen.ensure_set(fcall, typehint=rtype, path_id=path_id, ctx=ctx)
@@ -592,6 +598,7 @@ def compile_operator(
         typeref=typegen.type_to_typeref(rtype, env=env),
         typemod=oper.get_return_typemod(env.schema),
         tuple_path_ids=[],
+        impl_is_strict=oper.get_impl_is_strict(env.schema),
     )
 
     _check_free_shape_op(node, ctx=ctx)
@@ -860,7 +867,8 @@ def finalize_args(
                 arg, paramtype, srcctx=None, ctx=ctx)
 
         args.append(
-            irast.CallArg(expr=arg, expr_type_path_id=arg_type_path_id))
+            irast.CallArg(expr=arg, expr_type_path_id=arg_type_path_id,
+                          is_default=barg.is_default))
 
         # If we have any logged paths left over and our enclosing
         # context is logging paths, propagate them up.
diff --git a/edb/edgeql/compiler/polyres.py b/edb/edgeql/compiler/polyres.py
index f39c5434deb..552496808b4 100644
--- a/edb/edgeql/compiler/polyres.py
+++ b/edb/edgeql/compiler/polyres.py
@@ -48,6 +48,7 @@ class BoundArg(NamedTuple):
     val: irast.Set
     valtype: s_types.Type
     cast_distance: int
+    is_default: bool = False
 
 
 class MissingArg(NamedTuple):
@@ -428,6 +429,7 @@ def _get_cast_distance(
                         default,
                         param_type,
                         0,
+                        True,
                     )
                 )
 
diff --git a/edb/ir/ast.py b/edb/ir/ast.py
index 285d4bb9477..38dd91faac2 100644
--- a/edb/ir/ast.py
+++ b/edb/ir/ast.py
@@ -668,6 +668,7 @@ class CallArg(ImmutableBase):
     expr_type_path_id: typing.Optional[PathId] = None
     cardinality: qltypes.Cardinality = qltypes.Cardinality.UNKNOWN
     multiplicity: qltypes.Multiplicity = qltypes.Multiplicity.UNKNOWN
+    is_default: bool = False
 
 
 class Call(ImmutableExpr):
@@ -708,6 +709,11 @@ class Call(ImmutableExpr):
     # Volatility of the function or operator.
     volatility: qltypes.Volatility
 
+    # Whether the underlying implementation is strict in all its required
+    # arguments (NULL inputs lead to NULL results). If not, we need to
+    # filter at the call site.
+    impl_is_strict: bool = False
+
 
 class FunctionCall(Call):
 
diff --git a/edb/lib/std/25-booloperators.edgeql b/edb/lib/std/25-booloperators.edgeql
index 4387f17c208..9ebf4a0ccd1 100644
--- a/edb/lib/std/25-booloperators.edgeql
+++ b/edb/lib/std/25-booloperators.edgeql
@@ -21,39 +21,23 @@
 ## --------------------------
 
 
-# Unlike SQL, EdgeQL does not have the three-valued boolean logic,
-# and boolean operators must obey the same rules as all other
-# operators: they must yield an empty set if any of the operands
-# is an empty set, while in SQL `(True OR NULL) IS True`.
-# To achieve this, we convert the boolean op into an equivalent
-# bitwise OR expression (the shortest and fastest equivalent):
-#
-#    a OR b --> (a::int | b::int)::bool
-#
-# This transformation may break bitmap index scan optimization
-# when inside a WHERE clause, so we must use the original
-# boolean expression in conjunction.
 CREATE INFIX OPERATOR
 std::`OR` (a: std::bool, b: std::bool) -> std::bool {
     CREATE ANNOTATION std::identifier := 'or';
     CREATE ANNOTATION std::description := 'Logical disjunction.';
     SET volatility := 'Immutable';
-    USING SQL $$
-    SELECT ("a" OR "b") AND ("a"::int | "b"::int)::bool
-    $$;
+    SET impl_is_strict := false;
+    USING SQL EXPRESSION;
 };
 
 
-# For the same reasons as OR above, we need to make sure
-# False AND {} is {}.
 CREATE INFIX OPERATOR
 std::`AND` (a: std::bool, b: std::bool) -> std::bool {
     CREATE ANNOTATION std::identifier := 'and';
     CREATE ANNOTATION std::description := 'Logical conjunction.';
     SET volatility := 'Immutable';
-    USING SQL $$
-    SELECT ("a" AND "b") OR ("a"::int & "b"::int)::bool
-    $$;
+    SET impl_is_strict := false;
+    USING SQL EXPRESSION;
 };
 
 
diff --git a/edb/pgsql/common.py b/edb/pgsql/common.py
index 4e0d7bad373..18d1f825192 100644
--- a/edb/pgsql/common.py
+++ b/edb/pgsql/common.py
@@ -257,6 +257,8 @@ def get_pointer_backend_name(id, module_name, *, catenate=False, aspect=None):
 
 
 _operator_map = {
+    s_name.name_from_string('std::AND'): 'AND',
+    s_name.name_from_string('std::OR'): 'OR',
     s_name.name_from_string('std::NOT'): 'NOT',
     s_name.name_from_string('std::?='): 'IS NOT DISTINCT FROM',
     s_name.name_from_string('std::?!='): 'IS DISTINCT FROM',
diff --git a/edb/pgsql/compiler/expr.py b/edb/pgsql/compiler/expr.py
index a63b5059a38..45613f7c92f 100644
--- a/edb/pgsql/compiler/expr.py
+++ b/edb/pgsql/compiler/expr.py
@@ -315,6 +315,24 @@ def compile_SliceIndirection(
     return result
 
 
+def _compile_call_args(
+    expr: irast.Call, *,
+    ctx: context.CompilerContextLevel
+) -> List[pgast.BaseExpr]:
+    args = [dispatch.compile(a.expr, ctx=ctx) for a in expr.args]
+    for ref, ir_arg, typemod in zip(args, expr.args, expr.params_typemods):
+        if (
+            not expr.impl_is_strict
+            and ir_arg.cardinality.can_be_zero()
+            and ref.nullable
+            and typemod == ql_ft.TypeModifier.SingletonType
+        ):
+            raise RuntimeError(
+                'operations on potentially empty arguments not supported in '
+                'simple expressions')
+    return args
+
+
 @dispatch.compile.register(irast.OperatorCall)
 def compile_OperatorCall(
         expr: irast.OperatorCall, *,
@@ -346,7 +364,7 @@ def compile_OperatorCall(
             f'set returning operator {expr.func_shortname!r} is not supported '
             f'in simple expressions')
 
-    args = [dispatch.compile(a.expr, ctx=ctx) for a in expr.args]
+    args = _compile_call_args(expr, ctx=ctx)
     return compile_operator(expr, args, ctx=ctx)
 
 
@@ -579,7 +597,7 @@ def compile_FunctionCall(
         raise RuntimeError(
             'set returning functions are not supported in simple expressions')
 
-    args = [dispatch.compile(a.expr, ctx=ctx) for a in expr.args]
+    args = _compile_call_args(expr, ctx=ctx)
 
     if expr.has_empty_variadic and expr.variadic_param_type is not None:
         var = pgast.TypeCast(
@@ -713,13 +731,9 @@ def _compile_set_in_singleton_mode(
             ptr_stor_info = pg_types.get_ptrref_storage_info(
                 ptrref, resolve_type=False)
 
-            colref = pgast.ColumnRef(name=[ptr_stor_info.column_name])
-        elif irtyputils.is_scalar(node.typeref):
             colref = pgast.ColumnRef(
-                name=[
-                    common.edgedb_name_to_pg_name(str(node.typeref.id))
-                ]
-            )
+                name=[ptr_stor_info.column_name],
+                nullable=node.rptr.dir_cardinality.can_be_zero())
         else:
             colref = pgast.ColumnRef(
                 name=[
diff --git a/edb/pgsql/compiler/relgen.py b/edb/pgsql/compiler/relgen.py
index 7e60c2dec49..ea83d1f89b9 100644
--- a/edb/pgsql/compiler/relgen.py
+++ b/edb/pgsql/compiler/relgen.py
@@ -397,6 +397,9 @@ def _get_set_rvar(
             # {<const>[, <const> ...]}
             rvars = process_set_as_const_set(ir_set, stmt, ctx=ctx)
 
+        elif isinstance(expr, irast.OperatorCall):
+            rvars = process_set_as_oper_expr(ir_set, stmt, ctx=ctx)
+
         else:
             # All other expressions.
             rvars = process_set_as_expr(ir_set, stmt, ctx=ctx)
@@ -1902,6 +1905,24 @@ def process_set_as_const_set(
     return new_stmt_set_rvar(ir_set, stmt, ctx=ctx)
 
 
+def process_set_as_oper_expr(
+        ir_set: irast.Set, stmt: pgast.SelectStmt, *,
+        ctx: context.CompilerContextLevel) -> SetRVars:
+    expr = ir_set.expr
+    assert isinstance(expr, irast.OperatorCall)
+
+    # XXX: do we need a subrel?
+    with ctx.new() as newctx:
+        newctx.expr_exposed = False
+        args = _compile_call_args(ir_set, ctx=newctx)
+        oper_expr = exprcomp.compile_operator(expr, args, ctx=newctx)
+
+    pathctx.put_path_value_var_if_not_exists(
+        stmt, ir_set.path_id, oper_expr, env=ctx.env)
+
+    return new_stmt_set_rvar(ir_set, stmt, ctx=ctx)
+
+
 def process_set_as_expr(
         ir_set: irast.Set, stmt: pgast.SelectStmt, *,
         ctx: context.CompilerContextLevel) -> SetRVars:
@@ -2608,20 +2629,34 @@ def _compile_func_epilogue(
     return new_stmt_set_rvar(ir_set, stmt, aspects=aspects, ctx=ctx)
 
 
-def _compile_func_args(
+def _compile_call_args(
         ir_set: irast.Set, *,
         ctx: context.CompilerContextLevel
 ) -> List[pgast.BaseExpr]:
     expr = ir_set.expr
-    assert isinstance(expr, irast.FunctionCall)
+    assert isinstance(expr, irast.Call)
 
     args = []
 
-    for ir_arg in expr.args:
+    for ir_arg, typemod in zip(expr.args, expr.params_typemods):
         arg_ref = dispatch.compile(ir_arg.expr, ctx=ctx)
         args.append(output.output_as_value(arg_ref, env=ctx.env))
+        if (
+            not expr.impl_is_strict
+            and ir_arg.cardinality.can_be_zero()
+            and not ir_arg.is_default
+            and arg_ref.nullable
+            and typemod == qltypes.TypeModifier.SingletonType
+        ):
+            ctx.rel.where_clause = astutils.extend_binop(
+                ctx.rel.where_clause,
+                pgast.NullTest(arg=arg_ref, negated=True)
+            )
 
-        if ir_arg.expr_type_path_id is not None:
+        if (
+            isinstance(expr, irast.FunctionCall)
+            and ir_arg.expr_type_path_id is not None
+        ):
             # Object type arguments are represented by two
             # SQL arguments: object id and object type id.
             # The latter is needed for proper overload
@@ -2635,7 +2670,11 @@ def _compile_func_args(
             )
             args.append(type_ref)
 
-    if expr.has_empty_variadic and expr.variadic_param_type is not None:
+    if (
+        isinstance(expr, irast.FunctionCall)
+        and expr.has_empty_variadic
+        and expr.variadic_param_type is not None
+    ):
         var = pgast.TypeCast(
             arg=pgast.ArrayExpr(elements=[]),
             type_name=pgast.TypeName(
@@ -2675,7 +2714,7 @@ def process_set_as_func_enumerate(
     with ctx.subrel() as newctx:
         with newctx.new() as newctx2:
             newctx2.expr_exposed = False
-            args = _compile_func_args(inner_func_set, ctx=newctx2)
+            args = _compile_call_args(inner_func_set, ctx=newctx2)
         func_name = get_func_call_backend_name(inner_func, ctx=newctx)
 
         set_expr = _process_set_func_with_ordinality(
@@ -2699,7 +2738,7 @@ def process_set_as_func_expr(
 
     with ctx.subrel() as newctx:
         newctx.expr_exposed = False
-        args = _compile_func_args(ir_set, ctx=newctx)
+        args = _compile_call_args(ir_set, ctx=newctx)
         name = get_func_call_backend_name(expr, ctx=newctx)
 
         if expr.typemod is qltypes.TypeModifier.SetOfType:
diff --git a/edb/schema/functions.py b/edb/schema/functions.py
index d6055149218..12ad2665b4e 100644
--- a/edb/schema/functions.py
+++ b/edb/schema/functions.py
@@ -740,6 +740,9 @@ class CallableObject(
     abstract = so.SchemaField(
         bool, default=False, inheritable=False, compcoef=0.909)
 
+    impl_is_strict = so.SchemaField(
+        bool, default=True, compcoef=0.4)
+
     def as_create_delta(
         self: CallableObjectT,
         schema: s_schema.Schema,
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index dfb8faf28f9..595af78b1ad 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -7088,9 +7088,7 @@ async def test_edgeql_ddl_describe_schema(self):
                     SET volatility := 'Immutable';
                     CREATE ANNOTATION std::description :=
                         'Logical conjunction.';
-                    USING SQL $$
-                        SELECT ("a" AND "b") OR ("a"::int & "b"::int)::bool
-                    $$;
+                    USING SQL EXPRESSION;
                 };
                 ''',
                 '''
@@ -8898,7 +8896,10 @@ async def test_edgeql_ddl_enum_05(self):
                  CREATE CONSTRAINT expression ON (
                      <str>.num != asdf2()
                  );
-                 CREATE INDEX ON (asdf(.color));
+                 # FIXME: An index on asdf(.color) isn't supported currently
+                 # because passing nullable arguments requires filtering.
+                 # CREATE INDEX ON (asdf(.color));
+                 CREATE INDEX ON (.color);
                  CREATE PROPERTY lol -> str {
                      SET default := asdf2();
                  }
diff --git a/tests/test_edgeql_select.py b/tests/test_edgeql_select.py
index bde1f49788c..06e69e90d91 100644
--- a/tests/test_edgeql_select.py
+++ b/tests/test_edgeql_select.py
@@ -6900,3 +6900,26 @@ async def test_edgeql_assert_fail_object_computed_02(self):
             await self.con.query("""
                 SELECT array_agg((SELECT User {m := Publication}))[{1000}].m;
             """)
+
+    async def test_edgeql_select_call_null_01(self):
+        # testing calls with null args
+        await self.con.execute('''
+            create function foo(x: str, y: int64) -> str USING (x);
+        ''')
+
+        await self.assert_query_result(
+            r'''
+            select BooleanTest {
+                name,
+                val,
+                x := foo(.name, .val)
+            } order by .name;
+            ''',
+            [
+                {'name': 'circle', 'val': 2, 'x': 'circle'},
+                {'name': 'hexagon', 'val': 4, 'x': 'hexagon'},
+                {'name': 'pentagon', 'val': None, 'x': None},
+                {'name': 'square', 'val': None, 'x': None},
+                {'name': 'triangle', 'val': 10, 'x': 'triangle'},
+            ],
+        )


Code review comments:
diff_hunk: @@ -315,6 +315,24 @@ def compile_SliceIndirection(
     return result
 
 
+def _compile_call_args(
+    expr: irast.Call, *,
+    ctx: context.CompilerContextLevel
+) -> List[pgast.BaseExpr]:
+    args = [dispatch.compile(a.expr, ctx=ctx) for a in expr.args]
+    for ref, ir_arg, typemod in zip(args, expr.args, expr.params_typemods):
+        if (
+            not expr.impl_is_strict
+            and ir_arg.cardinality.can_be_zero()
+            and ref.nullable
+            and typemod == ql_ft.TypeModifier.SingletonType
+        ):
+            raise RuntimeError(
path: edb/pgsql/compiler/expr.py
text: @user1:
Is this a temporary assertion?  If not, then this should be `errors.UnsupportedFeatureError`

@author:
I made it a RuntimeError because that's what all the similar errors are, but they should probably all be UnsupportedFeatureError

@user2:
Can this surface in a user query? If so, a RuntimeErorr would become an ISE. Maybe we cleanup non-ISE RuntimeError in this file in a separate commit as part of this PR?

diff_hunk: @@ -8898,7 +8896,10 @@ async def test_edgeql_ddl_enum_05(self):
                  CREATE CONSTRAINT expression ON (
                      <str>.num != asdf2()
                  );
-                 CREATE INDEX ON (asdf(.color));
+                 # FIXME: An index on asdf(.color) isn't supported currently
+                 # because passing nullable arguments requires filtering.
+                 # CREATE INDEX ON (asdf(.color));
+                 CREATE INDEX ON (.color);
path: tests/test_edgeql_ddl.py
text: @user1:
Unless you want to fix this as part of this PR, I'd create an x-fail test so that we don't forget about this.

@author:
I just fixed it instead