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

merged 5 commits into from
May 22, 2024

Conversation

dnwpark
Copy link
Contributor

@dnwpark dnwpark commented May 13, 2024 •
edited

related #7264

  • Catches aggregate calls in constraints and indexes earlier in the compilation process
    • Raises errors in migration create which were previously only caught on migrate
  • Fixes abstract constraints not raising errors when using aggregate functions
  • Adds more information in errors about aggregate functions

Given the schema:

  type User {
     name: str;
  }

  type HasTwoOrFewerUsers {
     users: User;
     constraint expression on (count(.users) <= 2);
  }

The following error is produced:

error: cannot use aggregate functions or operators in a non-aggregating constraint
   │
25 │      constraint expression on (count(.users) <= 2);
   │                                ^^^^^^^^^^^^^ error

@dnwpark dnwpark force-pushed the non-multi-aggregate-constraint branch 5 times, most recently from be8773d to 18638e5 Compare May 15, 2024 22:53
@dnwpark dnwpark force-pushed the non-multi-aggregate-constraint branch 2 times, most recently from 5fa6dc7 to 74ac34e Compare May 16, 2024 22:49
@dnwpark dnwpark force-pushed the non-multi-aggregate-constraint branch from 74ac34e to 10a7776 Compare May 16, 2024 22:52
@dnwpark dnwpark changed the base branch from master to simplify-populate-constraint-attrs May 16, 2024 22:53
Base automatically changed from simplify-populate-constraint-attrs to master May 21, 2024 14:26
@dnwpark dnwpark changed the title test Catch aggregate calls in constraints and indexes in ql compiler May 21, 2024
@dnwpark dnwpark marked this pull request as ready for review May 21, 2024 14:33
msullivan
msullivan reviewed May 22, 2024
View reviewed changes
edb/ir/utils.py
Comment on lines +532 to +533
calls = ast.find_children(ir, irast.Call, flt, terminate_early=True)
return next(iter(calls or []), None)
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

msullivan
msullivan approved these changes May 22, 2024
View reviewed changes
edb/pgsql/compiler/expr.py Outdated
Comment on lines 436 to 437
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

edb/ir/utils.py
Comment on lines +502 to +514
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'),
}
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.

tests/test_edgeql_ddl.py Outdated
'set returning operator std::DISTINCT is not supported '
'in singleton expressions'):
edgedb.SchemaDefinitionError,
"cannot use aggregate operator 'std::DISTINCT' "
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 operator", not "aggregate operator"

edb/schema/constraints.py
'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

@dnwpark dnwpark force-pushed the non-multi-aggregate-constraint branch from 473d322 to 19c3fcf Compare May 22, 2024 17:20
@dnwpark dnwpark merged commit 58af334 into master May 22, 2024
24 checks passed
@dnwpark dnwpark deleted the non-multi-aggregate-constraint branch May 22, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msullivan msullivan msullivan approved these changes

@aljazerzen aljazerzen Awaiting requested review from aljazerzen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dnwpark @msullivan