edgedb / rfcs

RFCs for major changes to EdgeDB
Apache License 2.0
35 stars 5 forks source link

RFC 1023: Adding Splats Syntax #77

Closed 1st1 closed 1 year ago

tailhook commented 1 year ago

Also maybe we should:

  1. Have a session config option or compilation flag that enables splats? So we enable that in REPL but users have to make extra steps to use them in app code.
  2. Prohibit splats in codegen? On one hand, it's convenient. But on the other hand they will eventually overfetch, so this is a bad habit to have. Also while property-only splats are maybe okay, link-including splats in codegen are almost surely a bad idea.
tailhook commented 1 year ago

Also can we do something like this work:

WITH U = (SELECT User { first_name, last_name })
SELECT U {
    U.*,
    friends: {U.*},
}

I.e. some way to not repeat yourself in the query. Splats are a tool for that exactly, but...

1st1 commented 1 year ago

@tailhook

Have a session config option or compilation flag that enables splats? So we enable that in REPL but users have to make extra steps to use them in app code.

I think it's a bad idea to gate any kind of syntax behind a flag. It will undoubtedly create a lot of confusion (why my query works here but not there). IMO this is a documentation issue.

Prohibit splats in codegen? On one hand, it's convenient. But on the other hand they will eventually overfetch, so this is a bad habit to have. Also while property-only splats are maybe okay, link-including splats in codegen are almost surely a bad idea.

Theoretically we could include a flag to our type descriptor that it included a splat...But I'm not sure that we should do this in 3.0. Maybe a request for this should come from the community.

WITH U = (SELECT User { first_name, last_name }) SELECT U { U., friends: {U.}, }

No, this wouldn't work because the U alias doesn't define a new type. The type of U is still User with all its properties. You depend on that property of EdgeQL in your query, btw, selecting the friends link via the U alias.

elprans commented 1 year ago

One thing that still needs an explicit mention/discussion is this pattern:

WITH
   U := User { last_name := str_upper(.last_name) }  
SELECT (
   U { * },          # will select the `last_name` computed

   U { User.* },     # should this select the non-computed version of `last_name`?
                     # or should it be equivalent to U { U.* }?
)

I think making Child { Parent.* } equivalent to "select all properties found in Parent, but as they are defined in Child" is safer, but there could be a use case of "escaping the computed"?

1st1 commented 1 year ago

should this select the non-computed version of last_name?

IMO no, it should include the computed one. All User.* does is expanding U { User.* } to U { first_name, last_name, email, etc }. In other words I'm looking at this from a "symbolic substitution" point of view (which is the most straightforward view possible, I guess)

tailhook commented 1 year ago

I think it's a bad idea to gate any kind of syntax behind a flag. It will undoubtedly create a lot of confusion (why my query works here but not there). IMO this is a documentation issue.

We always disagree on how much we can rely on the documentation. But I don't think there will be "lot of confusion", because it's super-simple to write an explanation in the error message:

>>> conn.query("SELECT User {*}");
CannotUseAsteriskError: usage of asterisks in the queries is disabled
| SELECT User {*}
               ^ --- please expand to specific attribute names
hint: Properties of the User: id, first_name, last_name
hint: Links of the User: friends, profile, publications
help: This feature is developed for exploration in shell and GUI tools. But it's
help: better to enumerate specific fields in application code when using client library.
help: Alternatively enable `allow_asterisks` option.
more info: http://edgedb.com/p/asterisk

You can even be lazy enough to put star, see exception and copy-paste all the properties from the exception message.

Theoretically we could include a flag to our type descriptor that it included a splat...But I'm not sure that we should do this in 3.0. Maybe a request for this should come from the community.

I think annotation would be good enough. Also as I've already said, we should have an annotation for * to suggest user trying ** in REPL.

No, this wouldn't work because the U alias doesn't define a new type. The type of U is still User with all its properties. You depend on that property of EdgeQL in your query, btw, selecting the friends link via the U alias.

Sure. My point is that do-not-repeat-yourself is another super-important function of splats that we have to add eventually. If we don't want that syntax maybe we can have a special constructor for that:

WITH FRAGMENT Common := {first_name, last_name}
SELECT User { Common.*, friends: { Common.* }}

One interesting way of how bare "fragments" (the terminology stolen from GraphQL) can be used is kinda "reverse codegen":

#[derive(Queryable)]
struct BaseUser {
    first_name: String,
    last_name: String,
}

#[derive(Queryable)]
struct User {
    #[edgedb(flatten)]
    common: BaseUser,
    friends: Vec<BaseUser>,
}

let users: Vec<User> = query!(conn, "
    WITH FRAGMENT BaseUser := <some subst syntax to get fields from `struct BaseUser`>
    SELECT User { BaseUser.*, friends: { BaseUser.* } }
").await?

(There is a number of reasons of why it's more convenient to define structure in Rust rather then using codegen from EdgeDB query to Rust code)

1st1 commented 1 year ago

I think annotation would be good enough. Also as I've already said, we should have an annotation for * to suggest user trying ** in REPL.

Yeah, with annotations clients will be able to implement whatever API they want, including having a client-level setting to ban splats.

Sure. My point is that do-not-repeat-yourself is another super-important function of splats that we have to add eventually. If we don't want that syntax maybe we can have a special constructor for that:

WITH FRAGMENT Common := {first_name, last_name} SELECT User { Common., friends: { Common. }}

Maybe we don't need that. We do want to add support of defining type and type aliases in the with clause. With that and with free objects you'd be able to do this:

with
  shape as type {
    first_name: str,
    last_name: str
  }
select {
  users: User { shape.* },
  super_users: SuperUser { shape.* }
}

To clarify, with free objects and aliasing in queries implemented the above will have to work. So maybe we won't need to further sugar this with fragments or any other additional syntax.


Lastly, maybe we should prohibit using splats on anything but types. E.g. this would be an error:

with u := (select User filter ...)
select User {
  u.*
}

the query could be fixed though:

with u := (select User filter ...)
select User {
  (typeof u).*
}
tailhook commented 1 year ago

Maybe we don't need that. We do want to add support of defining type and type aliases in the with clause.


with
 shape as type {
   first_name: str,
   last_name: str
 }

The issue I see here is that you have to restate the types of the fields. This probably defeats the purpose of not repeating yourself, because this will require syncing types here to types in the schema.

Lastly, maybe we should prohibit using splats on anything but types.

This has a problem with computeds:

WITH U := (SELECT User { full_name := .first_name ++ " " ++ .last_name })

SELECT {
  users_with_full_name := (SELECT User { U.* }),
  users_without_full_name := (SELECT User { User.* }),
}
elprans commented 1 year ago

The issue I see here is that you have to restate the types of the fields

We may consider allowing incomplete free types, e.g. with anytype:

with
 fragment as type {
   foo: anytype,
   bar: anytype,     
 }

and, possibly even allow eliding anytype:

with
  fragment as type {
    foo,
    bar,
  }

This has a problem with computeds:

No problem, as suggested above, (typeof U).* should still work. The reason here is that .* is a type operator, not a value operator (in the same vein as the infix IS or INSERT are).

msullivan commented 1 year ago

Does ** include __type__ or should we explicitly exempt that? It is a link, but also it has 15 properties and the user maybe care about name (but probably not, since name is already injected in a much more convenient way). (Properties of schema::ObjectType: ["final", "expr", "is_from_alias", "builtin", "is_compound_type", "id", "inherited_fields", "from_alias", "is_abstract", "internal", "computed_fields", "is_final", "abstract", "name", "compound_type"])

tailhook commented 1 year ago

No problem, as suggested above, (typeof U).* should still work. The reason here is that .* is a type operator, not a value operator (in the same vein as the infix IS or INSERT are).

Currently you can do something is U, it always returns false. So it looks like a bug, right? I mean either U is also a type, and you can do is U or, U is not a type and in this case is U is a compilation error.

tailhook commented 1 year ago

Does ** include __type__ or should we explicitly exempt that?

I think exemption is good. Probably SELECT (introspect typeof X).* is a way to find out details about the type.

elprans commented 1 year ago

Currently you can do something is U, it always returns false. So it looks like a bug, right? I mean either U is also a type, and you can do is U or, U is not a type and in this case is U is a compilation error.

This seems like a bug to me. @msullivan?

1st1 commented 1 year ago

Does ** include type or should we explicitly exempt that?

Definitely exempt.

Currently you can do something is U, it always returns false. So it looks like a bug, right? I mean either U is also a type, and you can do is U or, U is not a type and in this case is U is a compilation error.

This seems like a bug to me. @msullivan?

Yeah, sounds like this should have been a compile-time error.

elprans commented 1 year ago

@1st1, @msullivan, one thing we forgot about here are the usual middle children: link properties. I think ** should include them, but this deserves a mention in the RFC.

elprans commented 1 year ago

Another missing bit is the combination of polymorphic intersection and explicit type source, e.g. Foo[IS Bar].*

tailhook commented 1 year ago

Another missing bit is the combination of polymorphic intersection and explicit type source, e.g. Foo[IS Bar].*

Oh, there is actually two questions here:

  1. Whether Foo[IS Bar].* supported? I would say it's unlikely useful in ad-hoc repl queries. And we don't want asterisks in production queries.
  2. Whether Foo {*} expands to Foo { Foo.*, [IS Bar]Bar.*, [IS Baz]Baz.* }, which sounds as something hugely useful and nice.