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

RFC: Simplified filtering API #347

Closed
colinhacks opened this issue May 10, 2022 · 8 comments
Closed

RFC: Simplified filtering API #347

colinhacks opened this issue May 10, 2022 · 8 comments
Assignees

Comments

@colinhacks
Copy link
Contributor

colinhacks commented May 10, 2022

A proposal for simplified filter syntax for equality filters. The goal is to expand the scope of cardinality inference and provide a less verbose alternative to e.op(x, "=", y) when specifying simple equality filters.

In e.select, the filter key should also accept a plain JS object. The keys can correspond to an property/link name and can point to singleton expressions or plain JS literals.

Current syntax

e.select(e.Movie, (movie) => ({
  filter: e.op(movie.release_year, '=', '2008'),
}));

Proposed syntax

e.select(e.Movie, () => ({
  filter: {release_year: 1999},
}))

It's no longer necessary to include a reference to the scope variable movie or write the e.op() expression, which is one of the bits of query builder syntax that is hardest to type quickly and least conducive to autocompletion.

Type inference still works as expected. Filtering on a non-exclusive property returns an array:

e.select(e.Movie, (movie) => ({
  title: true,
  filter: {release_year: 1999},
})).run(client);
// {title: string}[]

If a referenced property/link has an exclusive constraint, the result will be a singleton.

e.select(e.Movie, (movie) => ({
  title: true,
  filter: {id: e.uuid('2053a8b4-49b1-437a-84c8-e1b0291ccd9f')},
}));
// {title: string}

This hold true for object-level exclusive constraints corresponding to a tuple of properties. (This is infeasible to implement using the e.op formulation.) e.g. if there is a compound constraint on (.title, .release_year) the following would be inferred to be a singleton.

// singleton
e.select(e.Movie, (movie) => ({
  title: true,
  filter: {
    title: "A Star is Born"
    release_year: 2018
  }
}));
// {title: string}

An additional proposal is to support cardinality inference for "expression constraints". e.g.

type Movie {
  property title -> str;
  constraint exclusive on str_lower(.title);
}

The constraint's stored expr would be introspected to support the following:

// singleton
e.select(e.Movie, (movie) => ({
  title: true,
  filter: {
    'str_lower(.title)': 'a star is born',
  },
}));

Non-exclusive

There is a question of whether the "object filter" syntax would support non-exclusive properties. @elprans objected to supporting this syntax exclusively for singleton/exclusive filtering. Filtering by equality on an exclusive vs nonexclusive property would look very different and possibly lead to confusion.

e.select(e.Movie, (,)=>({
  filter: { id: "adsf" }
  filter: e.op(m.rating, '=', 5),
}));

This proposal can be generalized to a way to specify any set of equality filters (#347)

e.select(e.Movie, ()=>({
  filter: { rating: 5 }  // non-exclusive prop
}));

This fixes the inconsistency, but makes it less visually obvious when a filter clause corresponds to a singleton filter. It also increases implementation complexity, since we'll need to statically determine whether keys of the "object filter" object correspond to an exclusive constraint, instead of simply checking if the value passed to filter is an object literal.

@colinhacks colinhacks self-assigned this May 10, 2022
@liron00
Copy link

liron00 commented May 31, 2022

Related as a further syntactic sugar: edgedb/edgedb#3810 (comment)

When writing a subquery select-by-id, it would be great if we had an even more compact syntax to refer to the linked entity by its ID without spelling out the query.

@ghost
Copy link

ghost commented Jun 2, 2022

What about other operators?

Personally, I prefer something close to Graphql's style:

FROM: filter: e.op(left, '>', right)

TO: filter: {left: {e.gt: right}}

FROM: filter: e.op(left, 'or', right)

TO: filter: {e.or: [left, right]}

...

@1st1
Copy link
Member

1st1 commented Jun 3, 2022

How would you combine new syntax and old if you have a bunch of eq filters and one non-eq? Would this be possible:

e.select(e.Movie, (movie) => ({
  title: true,
  filter: e.op({
    title: "A Star is Born"
    release_year: 2018
  }, 'and', e.op(movie.blah, '>', 10))
}));

?

Also, do we have to use e.uuid()? Can we allow ID types to accept string literals directly? The casting seems a bit too verbose and not super necessary.

@colinhacks
Copy link
Contributor Author

colinhacks commented Jun 6, 2022

Personally, I prefer something close to Graphql's style:

I don't hate this, it's familiar to people and has better autocompletion than e.op. But it also doesn't have the composability of e.op and requires several levels of object nesting, which I don't love. It's certainly on the table, but it's a separate discussion.

How would you combine new syntax and old if you have a bunch of eq filters and one non-eq? Would this be possible:

This syntax wouldn't be allowed under the current proposal since I'm not planning to overload e.op to accept these "filter objects". This is merely an extension to what can be passed into the filter expression in a select/update/delete. Previously you could only pass a boolean expression, now you can pass either a boolean expression or this special-case "object filter". I also don't think it's important for this syntax to be composable in e.op since it'll almost exclusively be used in conjunction with id or exclusive properties.


Both of these points make me think that my original proposal will be easier to explain/understand, namely that this "object filter" syntax should only work for exclusive properties or tuples.

@elprans argued in favor of expanding this syntax to include all properties, s.t. this is a general purpose syntactic sugar for specifying any equality filter. But personally I like the idea of a special syntax only for uniquely specifying a single object. It's makes it visually obvious that you are selecting a single object, which makes cardinality inference less mysterious. Providing this syntax to include all properties invites additional questions, including the two above: "How do I use other operators?" and "How do I compose this with other expressions?". Both of these potential points of confusion are eliminated if we limit to exclusive properties/tuples, since there's never a reason to use non-equality operators or do .id = "whatever" AND <expr> when selecting a single object. (Not for nothing, it's also would dramatically reduce implementation complexity.)

@colinhacks
Copy link
Contributor Author

do we have to use e.uuid()? Can we allow ID types to accept string literals directly? The casting seems a bit too verbose and not super necessary

We are doing this currently because we're being conservative and trying to keep the query builder semantics as close as possible to EdgeQL. Personally I agree this causes more confusion that it's worth.

@liron00
Copy link

liron00 commented Jun 6, 2022

String IDs without e.uuid seems good IMO, might just want to clarify the deal with the string allowed to have / not have dashes

@NetOpWibby
Copy link

Is there something blocking this from getting into EdgeDB?

@scotttrinh
Copy link
Collaborator

Implemented in the query builder: https://www.edgedb.com/docs/clients/js/querybuilder#select-a-single-object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants