search

edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
515 stars 66 forks source link

Stop doing assert_exists insertion for edgedb versions with the bug fixed #562

Open msullivan opened 1 year ago

msullivan commented 1 year ago

The assert_exists insertion for required multi links was introduced as a workaround for https://github.com/edgedb/edgedb-js/issues/333.

https://github.com/edgedb/edgedb/pull/5180 fixes that.

As soon as we can, we should try to stop doing this; assert_exists has nontrivial performance implications and a large surface for bugs. (Hopefully some of those I'll be fixing imminently but it's still going to be more trouble)

elprans commented 1 year ago

Hm, assert_exists should get elided for inferred cardinality of AT_LEAST_ONE, no?

msullivan commented 1 year ago

Yes and no. The SQL we would generate gets elided during pgsql, but it still has nontrivial impacts on the IR side (since we might have to recompile a shape on the outside of it). That can result in extra joins and has a larger bug surface.

It's nontrivial to do this optimization during edgeql->IR with the current architecture, because we can't infer cardinality until we are finished.

It might be possible to detect assert_exists on AT_LEAST_ONE expressions in a postprocessing phase and clean everything up. Another option would be to do a provisional cardinality inference during IR compilation on assert_exists calls, and use that. It might not be correct, but I /think/ it can only ever be "too high", not "too low", so the optimization would be sound. (Since it could be failing to take into account a path factoring, but I think that's it.)

It's not clear either of those approaches are worth it?

elprans commented 1 year ago

Hm, OK. Let's put this on a TODO list then. I remember you were musing some sort of larger refactor where inference is done in a pre-pass, which would seem to be the cleanest way forward.