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.

Sign up for GitHub

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

Jump to bottom

Catch aggregate calls in constraints and indexes in ql compiler #7343

Merged
merged 5 commits into from
May 22, 2024
Merged

Catch aggregate calls in constraints and indexes in ql compiler #7343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ad368d
Simplify populate concrete constraint attrs.
dnwpark May 16, 2024
a3bda07
Catch aggregate calls earlier in constraints.
dnwpark May 13, 2024
10a7776
Catch aggregate calls earlier in indexes.
dnwpark May 16, 2024
61862f5
Replace ... with pass.
dnwpark May 22, 2024
19c3fcf
Updated error message.
dnwpark May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions edb/ir/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
from edb.common import ordered

from edb.edgeql import qltypes as ft
from edb.schema import name as sn

from . import ast as irast
from . import typeutils
Expand Down Expand Up @@ -498,6 +499,49 @@ def flt(n: irast.Call) -> bool:
return bool(ast.find_children(ir, irast.Call, flt, terminate_early=True))


def is_singleton_set_of_call(
call: irast.Call
) -> bool:
# Some set functions and operators are allowed in singleton mode
# as long as their inputs are singletons

return call.func_shortname in {
sn.QualName('std', 'IN'),
sn.QualName('std', 'NOT IN'),
sn.QualName('std', 'EXISTS'),
sn.QualName('std', '??'),
sn.QualName('std', 'IF'),
}
Comment on lines +493 to +505
Copy link
Member

Choose a reason for hiding this comment

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

We could make this a new flag on operators that we configure in the standard library creation code. No sure if it is worth it, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially was going to do that, but it seemed like a lot of machinery. I think it's probably the "correct" thing to do in the long run.



def has_set_of_param(
call: irast.Call,
) -> bool:
return any(
arg.param_typemod == ft.TypeModifier.SetOfType
for arg in call.args.values()
)


def returns_set_of(
call: irast.Call,
) -> bool:
return call.typemod == ft.TypeModifier.SetOfType


def find_set_of_op(
ir: irast.Base,
has_multi_param: bool,
) -> Optional[irast.Call]:
def flt(n: irast.Call) -> bool:
return (
(has_multi_param or not is_singleton_set_of_call(n))
and (has_set_of_param(n) or returns_set_of(n))
)
calls = ast.find_children(ir, irast.Call, flt, terminate_early=True)
return next(iter(calls or []), None)
Comment on lines +532 to +533
Copy link
Member

Choose a reason for hiding this comment

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

Does the bool thing not actually work?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're trying to get the actual value, OK



T = TypeVar('T')


Expand Down
20 changes: 16 additions & 4 deletions edb/pgsql/compiler/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,10 +433,16 @@ def compile_OperatorCall(
dispatch.compile(r_expr, ctx=ctx),
],
)
elif expr.typemod is ql_ft.TypeModifier.SetOfType:
elif irutils.is_singleton_set_of_call(expr):
...
Copy link
Member

Choose a reason for hiding this comment

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

pass is traditional for an empty branch

elif irutils.returns_set_of(expr):
raise errors.UnsupportedFeatureError(
f'set returning operator {expr.func_shortname} is not supported '
f'in singleton expressions')
f"set returning operator '{expr.func_shortname}' is not supported "
f"in singleton expressions")
elif irutils.has_set_of_param(expr):
raise errors.UnsupportedFeatureError(
f"aggregate operator '{expr.func_shortname}' is not supported "
f"in singleton expressions")

args, maybe_null = _compile_call_args(expr, ctx=ctx)
return _wrap_call(
Expand Down Expand Up @@ -683,9 +689,15 @@ def compile_FunctionCall(
f'unimplemented function for singleton mode: {fname}'
)

if expr.typemod is ql_ft.TypeModifier.SetOfType:
if irutils.is_singleton_set_of_call(expr):
...
elif irutils.returns_set_of(expr):
raise errors.UnsupportedFeatureError(
'set returning functions are not supported in simple expressions')
elif irutils.has_set_of_param(expr):
raise errors.UnsupportedFeatureError(
f"aggregate function '{expr.func_shortname}' is not supported "
f"in singleton expressions")

args, maybe_null = _compile_call_args(expr, ctx=ctx)

Expand Down
20 changes: 14 additions & 6 deletions edb/schema/constraints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,12 +931,20 @@ def _populate_concrete_constraint_attrs(
span=sourcectx
)

if has_any_multi and ir_utils.contains_set_of_op(
final_subjectexpr.irast):
raise errors.InvalidConstraintDefinitionError(
"cannot use aggregate functions or operators "
"in a non-aggregating constraint",
span=sourcectx
if set_of_op := ir_utils.find_set_of_op(
final_subjectexpr.irast,
has_any_multi,
):
label = (
'function'
if isinstance(set_of_op, irast.FunctionCall) else
'operator'
)
op_name = str(set_of_op.func_shortname)
raise errors.UnsupportedFeatureError(
Copy link
Member

Choose a reason for hiding this comment

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

We should say SET OF instead of aggregate.

Also, I know I wrote the original error message, but I have no idea what "non-aggregating constraint" means, so we should stop saying it. :P

f"cannot use aggregate {label} '{op_name}' "
f"in a non-aggregating constraint",
span=set_of_op.span
)

if (
Expand Down
156 changes: 150 additions & 6 deletions tests/test_constraints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,9 +1400,9 @@ async def test_constraints_ddl_08(self):
""")

async with self.assertRaisesRegexTx(
edgedb.InvalidConstraintDefinitionError,
"cannot use aggregate functions or operators in a "
"non-aggregating constraint",
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::EXISTS' "
"in a non-aggregating constraint",
):
await self.con.execute("""
ALTER TYPE ObjCnstr2 {
Expand All @@ -1411,9 +1411,9 @@ async def test_constraints_ddl_08(self):
""")

async with self.assertRaisesRegexTx(
edgedb.InvalidConstraintDefinitionError,
"cannot use aggregate functions or operators in a "
"non-aggregating constraint",
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::EXISTS' "
"in a non-aggregating constraint",
):
await self.con.execute("""
ALTER TYPE ObjCnstr2 {
Expand Down Expand Up @@ -2119,3 +2119,147 @@ async def test_constraints_abstract_object_03(self):
await self.con.execute("""
update ChatBase set { messages := 'hello world' };
""")

async def test_constraints_singleton_set_ops_01(self):
await self.con.execute(
"""
create type X {
create property a -> int64 {
create constraint expression on (
__subject__ in {1}
);
}
};
""")

async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::IN' "
"in a non-aggregating constraint"
):
await self.con.execute(
"""
create type Y {
create multi property a -> int64 {
create constraint expression on (
__subject__ in {1}
);
}
};
""")

async def test_constraints_singleton_set_ops_02(self):
await self.con.execute(
"""
create type X {
create property a -> int64 {
create constraint expression on (
__subject__ not in {1}
);
}
};
""")

async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::NOT IN' "
"in a non-aggregating constraint"
):
await self.con.execute(
"""
create type Y {
create multi property a -> int64 {
create constraint expression on (
__subject__ not in {1}
);
}
};
""")

async def test_constraints_singleton_set_ops_03(self):
await self.con.execute(
"""
create type X {
create property a -> int64 {
create constraint expression on (
exists(__subject__)
);
}
};
""")

async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::EXISTS' "
"in a non-aggregating constraint"
):
await self.con.execute(
"""
create type Y {
create multi property a -> int64 {
create constraint expression on (
exists(__subject__)
);
}
};
""")

async def test_constraints_singleton_set_ops_04(self):
await self.con.execute(
"""
create type X {
create property a -> int64 {
create constraint expression on (
__subject__ ?? 1 = 0
);
}
};
""")

async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
r"cannot use aggregate operator 'std::\?\?' "
r"in a non-aggregating constraint"
):
await self.con.execute(
"""
create type Y {
create multi property a -> int64 {
create constraint expression on (
__subject__ ?? 1 = 0
);
}
};
""")

async def test_constraints_singleton_set_ops_05(self):
await self.con.execute(
"""
create type X {
create property a -> tuple<bool, int64> {
create constraint expression on (
__subject__.1 < 0
if __subject__.0 else
__subject__.1 >= 0
);
}
};
""")

async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
"cannot use aggregate operator 'std::IF' "
"in a non-aggregating constraint"
):
await self.con.execute(
"""
create type Y {
create multi property a -> tuple<bool, int64> {
create constraint expression on (
__subject__.1 < 0
if __subject__.0 else
__subject__.1 >= 0
);
}
};
""")
20 changes: 10 additions & 10 deletions tests/test_edgeql_ddl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10852,8 +10852,8 @@ async def test_edgeql_ddl_constraint_26(self):
async def test_edgeql_ddl_constraint_27(self):
async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
"cannot use aggregate operator 'std::DISTINCT' "
"in a non-aggregating constraint"):
await self.con.execute(r"""
CREATE TYPE default::ConstraintNonSingletonTest {
CREATE PROPERTY has_bad_constraint: std::str {
Expand All @@ -10867,8 +10867,8 @@ async def test_edgeql_ddl_constraint_27(self):
async def test_edgeql_ddl_constraint_28(self):
async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
"cannot use aggregate operator 'std::DISTINCT' "
"in a non-aggregating constraint"):
await self.con.execute(r"""
CREATE TYPE default::ConstraintNonSingletonTest {
CREATE PROPERTY has_bad_constraint: std::str {
Expand All @@ -10882,8 +10882,8 @@ async def test_edgeql_ddl_constraint_28(self):
async def test_edgeql_ddl_constraint_29(self):
async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
"cannot use aggregate operator 'std::DISTINCT' "
"in a non-aggregating constraint"):
await self.con.execute(r"""
CREATE TYPE default::ConstraintNonSingletonTest {
CREATE PROPERTY has_bad_constraint: std::str;
Expand All @@ -10896,8 +10896,8 @@ async def test_edgeql_ddl_constraint_29(self):
async def test_edgeql_ddl_constraint_30(self):
async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
"set returning operator 'std::DISTINCT' is not supported "
"in singleton expressions"):
await self.con.execute(r"""
CREATE TYPE default::ConstraintNonSingletonTest {
CREATE PROPERTY has_bad_constraint: std::str;
Expand All @@ -10909,8 +10909,8 @@ async def test_edgeql_ddl_constraint_30(self):
async def test_edgeql_ddl_constraint_31(self):
async with self.assertRaisesRegexTx(
edgedb.UnsupportedFeatureError,
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
"set returning operator 'std::DISTINCT' is not supported "
"in singleton expressions"):
await self.con.execute(r"""
CREATE ABSTRACT CONSTRAINT default::bad_constraint {
USING ((DISTINCT __subject__ = __subject__));
Expand Down