-
Notifications
You must be signed in to change notification settings - Fork 5
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
Pre-RFC: Simpler inserts #6
Comments
Thanks Paul, this is a good proposal.
Yeah, but we will have query builders, it's only a question of when. And even before query builders we can create a driver-level API to simplify inserts without the need to complicate EdgeQL/server/protocol. At least right now this seems the preferred option to me.
We planned to use |
Lastly, your opening example can be simplified: db.fetchone("""
INSERT User {
first_name := <str>$first_name,
last_name := <str>$last_name,
birth_date := <cal::local_date>$birth_date,
}
""", **form) And it's debatable if db.fetchone("""
INSERT User {
{...$form}
}
""", **form) is actually clearer and better. |
Thanks for starting the discussion, Paul!
No, the type cast operator semantically is a constructor and since objects can only be constructed by inserting, type casts with object types are not supported.
Yes, we are contemplating adding a generic spread operator in shapes: edgedb/edgedb#180
A question "how do I insert data as a blob of JSON?" has popped more than once, and we also want to make it easy to save an entire form of data, so it's worth exploring. Perhaps if a sub-dict in input has a non-empty "id" it's a nested Another open question is how do you actually describe the argument data and is it allowed to be non-homogeneous? |
Can you elaborate? Anything I can come up with, doesn't work with anything a little more complex than bare insert, like:
Sure. That's not the point. The point is that you have that enumeration in the app code anyway (either specifically for insert, as I've shown in example or in say API or form validation code), there is no reason to duplicate it.
That's also not a point. We just abuse cast as variable type specification. Cast isn't something that's inherent in variable usage, so we could invent any other syntax, like
Does it conflicts with this issue? For me it looks like it doesn't, but I'm not sure.
We can probably explore that theoretically, so we don't design thing that is too limited, but I would skip this in the first version of implementation.
This question is only important if we accept list of dicts as an argument. Or inserting the links. Answering the question: I would disallow different shapes in the first implementation. And then re-evaluate whenever there are users complaining. I believe that it's much harder to do, if we want to avoid recompiling postgres query on every request (perhaps, not impossible, though) |
Sure, I was clarifying the semantics of the cast syntax and why it can't be used for objects.
It doesn't, but it's worth keeping that perspective in mind. |
Also, in fact I have more complex examples in mind: db.fetchone("""
INSERT Session {...$session, user := (INSERT User {...$user})}
""", user=dict(...), session=dict(...)) This is much more tricky to do via |
Motivation
Insert statements are usually look like:
This is repetitive and error-prone (adding a field must take place in multiple places).
If some fields are optional, it needs the whole query builder to deal with that correctly.
(spectrum thread)
New Syntax
So I propose to make a dedicated syntax for that:
This is much easier to write and doesn't need query builder for optional fields.
It uses
..
operator that comes from Rust. We can use**
from python, or...
from Javascript.Protocol
This requires the new protocol message to make queries, let's call it
SmartExecute
which consists of:query
: EdgeQL textparams
: Data serialized by some self-descriptive serialization formatoutput_typedesc_id
: Optional, plays same role as inOptimisticExecute
Notes:
output_typedesc_id
is specified and it matches the value, this behaves as optimistic executeClient
Client might switch to this new protocol message if any of the argument is a dictionary. Otherwise, keep old protocol for compatibility. Later we can benchmark new protocol and make some other heuristics if the new protocol is faster (i.e. at least when there are no arguments).
Internals
params
and sends it to the compiler alongside withquery
. Compiler returns same thing as normal query, exceptargmap
is a bit more complex (contains not just indexes for top-level arguments but also for keys of dictionaries).Performance Notes
params
, which take a little bit more CPU but I doubt it's too much. Transcoding arguments in Rust should be quite fast.Open Questions and Alternatives
We use unpack/spread operator in proposal. We can use
<User>$x
instead, but:User {..$x, user_id=gen_user_id()}
and links.<SET OF User>$x
(as of Make query arguments required by default; add syntax to make them optional edgedb#1340)We may want to make type inference first (
INSERT { last_name :=$last_name }
), but this is not something that is strictly prerequisite and is not huge ergonomic improvement by itself.If this is a spread operator, should it work in places other than inserts? Perhaps yes, as far as we can make type inference for fields. Error "can't infer types, use expanded syntax" is fine for me.
Should this work for links? In the first implementation, no.
User {..$fields, link := (SELECT ..)}
is fine.The text was updated successfully, but these errors were encountered: