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

Coalesce DML express return more than one Element #6438

Closed
AnsonHwang86 opened this issue Nov 7, 2023 · 6 comments · Fixed by #6590
Closed

Coalesce DML express return more than one Element #6438

AnsonHwang86 opened this issue Nov 7, 2023 · 6 comments · Fixed by #6590

Comments

@AnsonHwang86
Copy link
Contributor

AnsonHwang86 commented Nov 7, 2023

  • EdgeDB Version: v4 on Cloud

1.defind schema

module default {
    type Foo {
	title: str;
        bar : Bar;
    }

    type Bar {
	title: str; 
    }

}

2.insert Foo

insert Foo {title:="Foo1"}
Here is the return value.
[
  {
    "id": "64682096-7d0a-11ee-8549-df022b184faa"
  }
]
  1. runing following edgedql
with data := <json>$data, 
upserts := (
  for item in json_array_unpack(data["bar"]) union (
    (update Bar filter .id = <uuid>json_get(item, "barId") set { title:=<str>json_get(item,"barTitle") })??
    (insert Bar { title:=<str>json_get(item,"barTitle") })
  )), 
update Foo filter .id = <uuid>data["fooId"] set {bar:=assert_single(upserts)}

Here is variable

{"fooId":"64682096-7d0a-11ee-8549-df022b184faa","bar":[{"barId":"1735ff7b-0a7c-40fa-9ac3-e5f1219ca9b7","barTitle":"bar1"}]}
** Pls note that the Bar of id "1735ff7b-0a7c-40fa-9ac3-e5f1219ca9b7" is not yet save in database yet, it was randomly provided by the customer
  1. by runing above, it will return a error
CardinalityViolationError: assert_single violation: more than one element returned by an expression

let's simply edgedb to show more exactly by removing array.

modify edgedb to

with data := <json>$data, 
upserts := (
 
    (update Bar filter .id = <uuid>json_get(data, "bar", "barId") set { title:=<str>json_get(data,"bar","barTitle") })??
    (insert Bar { title:=<str>json_get(data, "bar","barTitle") })
), 
update Foo filter .id = <uuid>data["fooId"] set {bar:=assert_single(upserts)}

here is data

{"fooId":"64682096-7d0a-11ee-8549-df022b184faa","bar":{"barId":"1735ff7b-0a7c-40fa-9ac3-e5f1219ca9b7","barTitle":"bar2"}}

Here is error

InternalServerError: more than one row returned by a subquery used as an expression

While I run

with data := <json>$data, 
upserts := (
 
    (update Bar filter .id = <uuid>json_get(data, "bar", "barId") set { title:=<str>json_get(data,"bar","barTitle") })??
    (insert Bar { title:=<str>json_get(data, "bar","barTitle") })
), 
select upserts

here is data

{"fooId":"64682096-7d0a-11ee-8549-df022b184faa","bar":{"barId":"1735ff7b-0a7c-40fa-9ac3-e5f1219ca9b7","barTitle":"bar2"}}

It did return only one element.
image

If I change id of Bar to one of id has existed in database, it will run correctly without this bug.
image

@AnsonHwang86
Copy link
Contributor Author

I notice there are serveral bug of Coalesce DML are fixed recently. Is this possible be fixed together? Thanks.

@msullivan
Copy link
Member

Fascinatingly, I think the bug here is actually in assert_single. See this failure:

create type Nully { create property x -> str; create property y -> str; };
insert Nully { y := 'x' };

Then both of these fail spuriously:

select Nully { xy := assert_single({.x, .y}) };
select Nully { xy := assert_single({.x, .x}) };

@msullivan
Copy link
Member

You can probably work around the issue by doing limit 1 instead of assert_single for now

msullivan added a commit that referenced this issue Dec 11, 2023
There are a number of places that improperly handle a set that has
multiple NULL values in it:
 * INSERT on single values
 * UPDATE on single values
 * assert_single

The assert_single case is a long standing bug that can be triggered
without much trouble. The INSERT/UPDATE bugs are new, and stem from
the new ?? on DML, since I think it that was the first thing in the
compiler backend where extra NULLS might be produced for something
with single cardinality.

Fix this by adding null checks at the consumer sites. One of the
consumer sites is set_as_subquery, and I modified update to always use
that.
I also added a helper function to add the null checks, and I'll be
going back and changing lots of other code to use the helper.

We could also flip this around a bit and try saying that we shouldn't
ever produce extra NULLs in a single set, and try to stamp it out on
the producer side, though that seems a little less consistent to me?

Fixes #6438.
@AnsonHwang86
Copy link
Contributor Author

AnsonHwang86 commented Dec 12, 2023

Great done.

You can probably work around the issue by doing limit 1 instead of assert_single for now

Yes, but it must following order by .id desc otherwise the limit 1 will get the empty set.

update Foo filter .id = <uuid>data["fooId"] set {bar:=(select upserts order by .id desc limit 1)}

msullivan added a commit that referenced this issue Dec 13, 2023
There are a number of places that improperly handle a set that has
multiple NULL values in it:
 * INSERT on single values produced by ??
 * UPDATE on single values produced by ??
 * assert_single
 * limit/offset

The assert_single and limit case is a long standing bug that can be triggered
without much trouble. The INSERT/UPDATE bugs are new, and stem from
the new ?? on DML, since I think it that was the first thing in the
compiler backend where extra NULLS might be produced for something
with single cardinality.

Fix the long-standing cases by adding null checks, but fix the
coalesce cases by adding a null check at the producer side.
The rule then, is that there can be NULLs in any produced set
that can be empty, but a single set has to return at most one row
(so it can't have extra NULLs).


Fixes #6438.
@msullivan
Copy link
Member

Yes, but it must following order by .id desc otherwise the limit 1 will get the empty set.

update Foo filter .id = <uuid>data["fooId"] set {bar:=(select upserts order by .id desc limit 1)}

Yup that was also a bug :(. Fixed in #5690 too.

aljazerzen pushed a commit that referenced this issue Dec 15, 2023
There are a number of places that improperly handle a set that has
multiple NULL values in it:
 * INSERT on single values produced by ??
 * UPDATE on single values produced by ??
 * assert_single
 * limit/offset

The assert_single and limit case is a long standing bug that can be triggered
without much trouble. The INSERT/UPDATE bugs are new, and stem from
the new ?? on DML, since I think it that was the first thing in the
compiler backend where extra NULLS might be produced for something
with single cardinality.

Fix the long-standing cases by adding null checks, but fix the
coalesce cases by adding a null check at the producer side.
The rule then, is that there can be NULLs in any produced set
that can be empty, but a single set has to return at most one row
(so it can't have extra NULLs).


Fixes #6438.
@AnsonHwang86
Copy link
Contributor Author

@msullivan #5690 is like a typo fix. Are you link a wrong issue?

aljazerzen pushed a commit that referenced this issue Dec 21, 2023
There are a number of places that improperly handle a set that has
multiple NULL values in it:
 * INSERT on single values produced by ??
 * UPDATE on single values produced by ??
 * assert_single
 * limit/offset

The assert_single and limit case is a long standing bug that can be triggered
without much trouble. The INSERT/UPDATE bugs are new, and stem from
the new ?? on DML, since I think it that was the first thing in the
compiler backend where extra NULLS might be produced for something
with single cardinality.

Fix the long-standing cases by adding null checks, but fix the
coalesce cases by adding a null check at the producer side.
The rule then, is that there can be NULLs in any produced set
that can be empty, but a single set has to return at most one row
(so it can't have extra NULLs).


Fixes #6438.
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.

2 participants