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.

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

Consider allowing DML in conditionals #4437

Closed
msullivan opened this issue Sep 27, 2022 · 4 comments · Fixed by #6181
Closed

Consider allowing DML in conditionals #4437

msullivan opened this issue Sep 27, 2022 · 4 comments · Fixed by #6181

Comments

@msullivan
Copy link
Member

msullivan commented Sep 27, 2022

I think we could do it without too much trouble.
The key insight here is that we actually already do support conditional DML: DML can appear inside of FOR statements, and so the number of times a DML statement is executed (including possibly zero) depends on the value of the for iterator.

Then,

IF_BRANCH if COND else ELSE_BRANCH

is (modulo the fact that it adds a fence around COND and potentially increases the inferred cardinality)
equivalent to

for b in COND union (
  {
    (for _ in (select () filter b) union (IF_BRANCH)),
    (for _ in (select () filter not b) union (ELSE_BRANCH)),
  }
)
@msullivan msullivan added edgeql under discussion Not ready for implementation compiler labels Sep 27, 2022
@elprans
Copy link
Member

elprans commented Sep 27, 2022

Also, COND must not be correlated with anything, as is the general rule for DML dependencies.

@msullivan
Copy link
Member Author

msullivan commented Jan 20, 2023

And if we don't do this, we should document this sort of pattern somewhere

(But let's just do it)

@avedetvedea
Copy link
Contributor

@msullivan I have used your for-loop workaround that you provided in another thread to do some pretty complicated things and never hit a bug. The only problem is I have crazy for loops in my queries when I want to do conditional-DMLs. Looking forward to having this feature which is so coherent with the EQL's do-all-in-a-single-query ethos

@AnsonHwang86
Copy link
Contributor

Waiting for this crazy pattern.

@msullivan msullivan self-assigned this Sep 27, 2023
msullivan added a commit that referenced this issue Sep 28, 2023
As we've discussed (and, tragically, recommended to users), we can
rewrite the conditionals into for loops:
    for b in COND union (
      {
	(for _ in (select () filter b) union (IF_BRANCH)),
	(for _ in (select () filter not b) union (ELSE_BRANCH)),
      }
    )

The main fiddly part is preserving the correct
cardinality/multiplicity inference. I did this by adding a
card_inference_override field to Set that specifies another set that
should determine the cardinality.

I don't love this, but I haven't thought of a cleaner approach that
doesn't give up the benefits of the desugaring approach.

We need more testing but I wanted to get something up for people to
look at / we can catch up on testing after the feature freeze if
needed.

Fixes #4437.
msullivan added a commit that referenced this issue Sep 28, 2023
As we've discussed (and, tragically, recommended to users), we can
rewrite the conditionals into for loops:
    for b in COND union (
      {
	(for _ in (select () filter b) union (IF_BRANCH)),
	(for _ in (select () filter not b) union (ELSE_BRANCH)),
      }
    )

The main fiddly part is preserving the correct
cardinality/multiplicity inference. I did this by adding a
card_inference_override field to Set that specifies another set that
should determine the cardinality.

I don't love this, but I haven't thought of a cleaner approach that
doesn't give up the benefits of the desugaring approach.

We need more testing but I wanted to get something up for people to
look at / we can catch up on testing after the feature freeze if
needed.

Fixes #4437.

* Support writing a bare {} in the else branch

Because of how casts are inserted in the func code, this required some
tweaking to casts:
 - The empty array cast needs to be able to look through a Set in
   order to be efficient
 - The empty set to object cast needs to be able to look through a
   Set in order to not generate an IR cast that messes things up
   because it doesn't provide a source and thus causes the #3030
   overlay issue to pop up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants