Repository: geldata/gel
Pull request: Fix slicing with an empty set
fixes #4327

Problem statement:
Slicing behaves incorrectly on NULL empty sets
Another instance of a classic edgedb bug.

Slicing is a regular (non-optional) operation, and so empty set inputs should result in an empty set output, like such:
```
edgedb> select [1,2,3][1:<int64>{}];
[]
```

But empty sets that get represented as NULL produce output as if the argument was omitted:
```
edgedb> select [1,2,3][1:<optional int64>$0];
Parameter <int64>$0 (Ctrl+D for empty set `{}`): 
[[2, 3]]
```

The root of the issue is that all slicing (missing arguments or not) is done by calling a `_slice` function, and we represent missing bounds as NULL.

Hints from issue discussion:
And json slicing behaves incorrectly on NULL subjects:
```
edgedb> select (<optional json>$0)[0:];
Parameter <json>$0 (Ctrl+D for empty set `{}`): 
edgedb error: InvalidValueError: cannot slice JSON UNKNOWN
  Hint: Slicing is only available for JSON arrays and strings.
```

Commit message: touch catalog version

Patch to review:
diff --git a/edb/buildmeta.py b/edb/buildmeta.py
index 90cb857e6aa..a511fcb5d1a 100644
--- a/edb/buildmeta.py
+++ b/edb/buildmeta.py
@@ -44,7 +44,7 @@
 
 
 # Increment this whenever the database layout or stdlib changes.
-EDGEDB_CATALOG_VERSION = 2022_08_19_00_00
+EDGEDB_CATALOG_VERSION = 2022_09_20_00_00
 EDGEDB_MAJOR_VERSION = 3
 
 
diff --git a/edb/pgsql/compiler/expr.py b/edb/pgsql/compiler/expr.py
index 30e8edd7110..f7bd0d30603 100644
--- a/edb/pgsql/compiler/expr.py
+++ b/edb/pgsql/compiler/expr.py
@@ -276,29 +276,37 @@ def compile_IndexIndirection(
 
 @dispatch.compile.register(irast.SliceIndirection)
 def compile_SliceIndirection(
-        expr: irast.SliceIndirection, *,
-        ctx: context.CompilerContextLevel) -> pgast.BaseExpr:
+    expr: irast.SliceIndirection, *, ctx: context.CompilerContextLevel
+) -> pgast.BaseExpr:
+
     # Handle Expr[Index], where Expr may be std::str, array<T> or
     # std::json. For strings we translate this into substr calls.
     # Arrays use the native slice syntax. JSON is handled by a
     # combination of unnesting aggregation and array slicing.
     with ctx.new() as subctx:
+
+        # Postgres arrays use 32bit indexes, so EdgeDB inherits this limitation
+        def compile_and_cast_int(expr):
+            expr = dispatch.compile(expr, ctx=subctx)
+            return pgast.TypeCast(arg=expr, type_name=pgast.TypeName(name=("integer",)))
+
         subctx.expr_exposed = False
         subj = dispatch.compile(expr.expr, ctx=subctx)
+
         if expr.start is None:
-            start: pgast.BaseExpr = pgast.NullConstant()
+            start: pgast.BaseExpr = pgast.LiteralExpr(expr="0")
         else:
-            start = dispatch.compile(expr.start, ctx=subctx)
+            start = compile_and_cast_int(expr.start)
+
         if expr.stop is None:
-            stop: pgast.BaseExpr = pgast.NullConstant()
+            # Max index in EdgeQL is 2^32-2 i.e. 2147483646
+            # because during conversion to Postgres index, we 
+            # add 1 to the stop and then subtract one.
+            stop: pgast.BaseExpr = pgast.LiteralExpr(expr="2147483646")
         else:
-            stop = dispatch.compile(expr.stop, ctx=subctx)
-
-    result = pgast.FuncCall(
-        name=('edgedb', '_slice'),
-        args=[subj, start, stop]
-    )
+            stop = compile_and_cast_int(expr.stop)
 
+    result = pgast.FuncCall(name=("edgedb", "_slice"), args=[subj, start, stop])
     return result
 
 
diff --git a/edb/pgsql/metaschema.py b/edb/pgsql/metaschema.py
index e3508044580..29883fe2659 100644
--- a/edb/pgsql/metaschema.py
+++ b/edb/pgsql/metaschema.py
@@ -1948,26 +1948,14 @@ class ArraySliceFunction(dbops.Function):
     """Get an array slice."""
     text = '''
         SELECT
-            CASE
-                WHEN start IS NULL THEN
-                    val[:edgedb._normalize_array_index(
-                            stop, array_upper(val, 1)) - 1]
-                WHEN stop IS NULL THEN
-                    val[edgedb._normalize_array_index(
-                            start, array_upper(val, 1)):]
-                ELSE
-                    val[edgedb._normalize_array_index(
-                            start, array_upper(val, 1)):
-                        edgedb._normalize_array_index(
-                            stop, array_upper(val, 1)) - 1]
-            END
+            val[edgedb._normalize_array_index(start, cardinality(val)):
+                edgedb._normalize_array_index(stop, cardinality(val)) - 1]
     '''
 
     def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_slice'),
-            args=[('val', ('anyarray',)), ('start', ('bigint',)),
-                  ('stop', ('bigint',))],
+            args=[('val', ('anyarray',)), ('start', ('int',)), ('stop', ('int',))],
             returns=('anyarray',),
             volatility='immutable',
             text=self.text,
@@ -2050,8 +2038,7 @@ class SubstrProxyFunction(dbops.Function):
     def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_substr'),
-            args=[('val', ('anyelement',)), ('start', ('bigint',)),
-                  ('length', ('int',))],
+            args=[('val', ('anyelement',)), ('start', ('int',)), ('length', ('int',))],
             returns=('anyelement',),
             volatility='immutable',
             strict=True,
@@ -2094,39 +2081,22 @@ class StringSliceImplFunction(dbops.Function):
     """Get a string slice."""
     text = r'''
         SELECT
-            CASE
-                WHEN start IS NULL THEN
-                    edgedb._substr(
-                        val,
-                        1,
-                        edgedb._normalize_array_index(
-                            stop, edgedb._length(val)) - 1
-                    )
-                WHEN stop IS NULL THEN
-                    substr(
-                        val,
-                        edgedb._normalize_array_index(
-                            start, edgedb._length(val))
-                    )
-                ELSE
-                    edgedb._substr(
-                        val,
-                        edgedb._normalize_array_index(
-                            start, edgedb._length(val)),
-                        edgedb._normalize_array_index(
-                            stop, edgedb._length(val)) -
-                        edgedb._normalize_array_index(
-                            start, edgedb._length(val))
-                    )
-            END
+            edgedb._substr(
+                val,
+                edgedb._normalize_array_index(
+                    start, edgedb._length(val)),
+                edgedb._normalize_array_index(
+                    stop, edgedb._length(val)) -
+                edgedb._normalize_array_index(
+                    start, edgedb._length(val))
+            )
     '''
 
     def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_str_slice'),
             args=[
-                ('val', ('anyelement',)),
-                ('start', ('bigint',)), ('stop', ('bigint',))
+                ('val', ('anyelement',)), ('start', ('int',)), ('stop', ('int',))
             ],
             returns=('anyelement',),
             volatility='immutable',
@@ -2143,8 +2113,7 @@ def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_slice'),
             args=[
-                ('val', ('text',)),
-                ('start', ('bigint',)), ('stop', ('bigint',))
+                ('val', ('text',)), ('start', ('int',)), ('stop', ('int',))
             ],
             returns=('text',),
             volatility='immutable',
@@ -2161,8 +2130,7 @@ def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_slice'),
             args=[
-                ('val', ('bytea',)),
-                ('start', ('bigint',)), ('stop', ('bigint',))
+                ('val', ('bytea',)), ('start', ('int',)), ('stop', ('int',))
             ],
             returns=('bytea',),
             volatility='immutable',
@@ -2296,18 +2264,19 @@ class JSONSliceFunction(dbops.Function):
     """Get a JSON array slice."""
     text = r'''
         SELECT
-            CASE jsonb_typeof(val)
-            WHEN 'array' THEN (
-                to_jsonb(_slice(
+            CASE
+            WHEN val IS NULL THEN NULL
+            WHEN jsonb_typeof(val) = 'array' THEN (
+                to_jsonb(edgedb._slice(
                     (
-                        SELECT array_agg(value)
+                        SELECT coalesce(array_agg(value), '{}'::jsonb[])
                         FROM jsonb_array_elements(val)
                     ),
                     start, stop
                 ))
             )
-            WHEN 'string' THEN (
-                to_jsonb(_slice(val#>>'{}', start, stop))
+            WHEN jsonb_typeof(val) = 'string' THEN (
+                to_jsonb(edgedb._slice(val#>>'{}', start, stop))
             )
             ELSE
                 edgedb.raise(
@@ -2328,8 +2297,7 @@ class JSONSliceFunction(dbops.Function):
     def __init__(self) -> None:
         super().__init__(
             name=('edgedb', '_slice'),
-            args=[('val', ('jsonb',)), ('start', ('bigint',)),
-                  ('stop', ('bigint',))],
+            args=[('val', ('jsonb',)), ('start', ('int',)), ('stop', ('int',))],
             returns=('jsonb',),
             # Same volatility as to_jsonb (stable)
             volatility='stable',
diff --git a/tests/test_edgeql_select.py b/tests/test_edgeql_select.py
index 8df053c4713..8aa1ad8c0d9 100644
--- a/tests/test_edgeql_select.py
+++ b/tests/test_edgeql_select.py
@@ -5171,6 +5171,98 @@ async def test_edgeql_select_slice_03(self):
             }],
         )
 
+    async def test_edgeql_select_slice_04(self):
+
+        await self.assert_query_result(
+            r"""
+            select [1,2,3,4,5][1:];
+            """,
+            [[2, 3, 4, 5]],
+        )
+
+        await self.assert_query_result(
+            r"""
+            select [1,2,3,4,5][:3];
+            """,
+            [[1, 2, 3]],
+        )
+
+        await self.assert_query_result(
+            r"""
+            select [1,2,3][1:<int64>{}];
+            """,
+            [],
+        )
+
+        # try to trick the compiler and to pass NULL into edgedb._slice
+        await self.assert_query_result(
+            r"""
+            select [1,2,3][1:<optional int64>$0];
+            """,
+            [],
+            variables=(None,),
+        )
+
+        self.assertEqual(
+            await self.con.query(
+                r"""
+                select to_json('[true, 3, 4, null]')[1:];
+                """
+            ),
+            edgedb.Set(('[3, 4, null]',))
+        )
+
+        self.assertEqual(
+            await self.con.query(
+                r"""
+                select to_json('[true, 3, 4, null]')[:2];
+                """
+            ),
+            edgedb.Set(('[true, 3]',))
+        )
+
+        await self.assert_query_result(
+            r"""
+            select (<optional json>$0)[2:];
+            """,
+            [],
+            variables=(None,),
+        )
+
+        self.assertEqual(
+            await self.con.query(
+                r"""
+                select to_json('"hello world"')[2:];
+                """
+            ),
+            edgedb.Set(('"llo world"',))
+        )
+
+        self.assertEqual(
+            await self.con.query(
+                r"""
+                select to_json('"hello world"')[:4];
+                """
+            ),
+            edgedb.Set(('"hell"',))
+        )
+
+        await self.assert_query_result(
+            r"""
+            select (<array<str>>[])[0:];
+            """,
+            [[]]
+        )
+
+        await self.assert_query_result(
+            r"""
+            select to_json('[]')[0:];
+            """,
+            [[]]
+        )
+
+
+
     async def test_edgeql_select_tuple_01(self):
         await self.assert_query_result(
             r"""


Code review comments:
diff_hunk: @@ -276,29 +276,37 @@ def compile_IndexIndirection(
 
 @dispatch.compile.register(irast.SliceIndirection)
 def compile_SliceIndirection(
-        expr: irast.SliceIndirection, *,
-        ctx: context.CompilerContextLevel) -> pgast.BaseExpr:
+    expr: irast.SliceIndirection, *, ctx: context.CompilerContextLevel
+) -> pgast.BaseExpr:
+
     # Handle Expr[Index], where Expr may be std::str, array<T> or
     # std::json. For strings we translate this into substr calls.
     # Arrays use the native slice syntax. JSON is handled by a
     # combination of unnesting aggregation and array slicing.
     with ctx.new() as subctx:
+
+        # Postgres arrays use 32bit indexes, so EdgeDB inherits this limitation
+        def compile_and_cast_int(expr):
+            expr = dispatch.compile(expr, ctx=subctx)
+            return pgast.TypeCast(arg=expr, type_name=pgast.TypeName(name=("integer",)))
+
         subctx.expr_exposed = False
         subj = dispatch.compile(expr.expr, ctx=subctx)
+
         if expr.start is None:
-            start: pgast.BaseExpr = pgast.NullConstant()
+            start: pgast.BaseExpr = pgast.LiteralExpr(expr="0")
         else:
-            start = dispatch.compile(expr.start, ctx=subctx)
+            start = compile_and_cast_int(expr.start)
+
         if expr.stop is None:
-            stop: pgast.BaseExpr = pgast.NullConstant()
+            # Max index in EdgeQL is 2^32-2 i.e. 2147483646
+            # because during conversion to Postgres index, we 
+            # add 1 to the stop and then subtract one.
+            stop: pgast.BaseExpr = pgast.LiteralExpr(expr="2147483646")
path: edb/pgsql/compiler/expr.py
text: @user1:
I'd compute the number instead of just writing it in

diff_hunk: @@ -276,29 +276,37 @@ def compile_IndexIndirection(
 
 @dispatch.compile.register(irast.SliceIndirection)
 def compile_SliceIndirection(
-        expr: irast.SliceIndirection, *,
-        ctx: context.CompilerContextLevel) -> pgast.BaseExpr:
+    expr: irast.SliceIndirection, *, ctx: context.CompilerContextLevel
+) -> pgast.BaseExpr:
+
     # Handle Expr[Index], where Expr may be std::str, array<T> or
     # std::json. For strings we translate this into substr calls.
     # Arrays use the native slice syntax. JSON is handled by a
     # combination of unnesting aggregation and array slicing.
     with ctx.new() as subctx:
+
+        # Postgres arrays use 32bit indexes, so EdgeDB inherits this limitation
+        def compile_and_cast_int(expr):
path: edb/pgsql/compiler/expr.py
text: @user1:
You'll need to put a type annotation on this for mypy to pass.
(mypy is pretty fast, and useful, so you'll probably want to run that locally even during development)