edgedb-js icon indicating copy to clipboard operation
edgedb-js copied to clipboard

Implement variadic version of `e.op`

Open elprans opened this issue 2 years ago • 15 comments

Currently, e.op is either unary, binary, or ternary and there is no easy way to combine a variadic number of operands using the same operator. This is frequently needed to, e.g., combine a bunch of filter expressions into an and chain.

I propose adding e.gather_op('<op>', [operands]) as a way to do that. For example:

const selectionSet = {title: true} as const;

const query = e.select(e.Movie, movie => {
  const filters = [
    e.op(movie.title, "ilike", "%avengers%"),
  ];

  if (genre) {
    filters.push(e.op(movie.genre, "in", e.Genre.RomCom));
  }

  return {
    ...selectionSet,
    filter: e.gather_op("and", filters),
  };
});
const result = await query.run(client);

@colinhacks also suggests adding a variadic e.and() sugar, because this is a very common pattern.

elprans avatar Feb 23 '22 19:02 elprans

apply_op is closer to JS idioms.

1st1 avatar Feb 23 '22 19:02 1st1

The issue here is that there's a difference between what all() does and what e.gather_op('and', ...) would do.

  • all() is a semantic construct, that extends the boolean and operator to a set. It's part of EdgeQL.
  • e.gather_op() is a syntactic construct. It literally is a shorthand for the syntax of writing a bunch of expressions connected with and. It's barely more sophisticated than '(' + expsrs_list.join(') and (') + ')'. Alternatively you could view it as an ad-hoc element-wise function, but since we don't have those in EdgeQL, the construct is still firmly in the realm of query-builder, not EdgeQL itself. It is syntax sugar that currently only exists outside of EdgeQL. (Adding it to EdgeQL is tricky because it would introduce subtle confusion between this multi and and all that may be a bigger problem than the one it solves)

I'm against sugar (e.and()) on top of sugar (generic e.gather_op()) as that tends to make the API too complicated and heterogeneous.

vpetrovykh avatar Feb 23 '22 19:02 vpetrovykh

apply isn't exactly right, because we don't "apply" and to each argument, it's more like "join".

elprans avatar Feb 23 '22 19:02 elprans

I'm against sugar (e.and()) on top of sugar

I'm also a bit wary of adding e.and() sugar right away. IMO we should stick to the "generic first, sugar later" approach.

elprans avatar Feb 23 '22 19:02 elprans

apply isn't exactly right, because we don't "apply" and to each argument, it's more like "join".

Maybe we should call it e.join_op() to emphasize that this is basically syntactically stitching stuff with an operator?

vpetrovykh avatar Feb 23 '22 19:02 vpetrovykh

Oh, wait... maybe "join" is a bad and confusing keyword in the database context.

vpetrovykh avatar Feb 23 '22 19:02 vpetrovykh

Oh, wait... maybe "join" is a bad and confusing keyword in the database context.

Exactly.

elprans avatar Feb 23 '22 20:02 elprans

Perhaps e.op_all or e.op_many?

Agreed w.r.t e.and - retracted.

colinhacks avatar Mar 01 '22 22:03 colinhacks

I would avoid all in the name for this construct to distinguish it from the all builtin as much as possible.

vpetrovykh avatar Mar 02 '22 17:03 vpetrovykh

Haven't seen any suggestion for e.combine_op yet. Or maybe rather e.op_combine for autosuggest closeness.

Also how about pluralizing it? e.combine_ops?

tdolsen avatar Mar 19 '22 09:03 tdolsen

I came here because the EdgeDB AI docs suggested me to use e.and([e.op(), e.op(), e.op()]). After a little more prompting, it told me that e.and() did in fact not exist 😅 Although it would have been great for my many nested e.op(...,'and',...)

dotlouis avatar Feb 19 '24 19:02 dotlouis

@dotlouis

FWIW, we do have something close to this in the form of e.all which would look like:

e.all(e.set([e.op(), e.op(), e.op()]));

Which uses the all standard library function to chain a set of booleans together.

all({a, b, c})

This is different from the proposed issue here which is wanting generate:

(a and b and c)

Stay tuned as we work on trying to pick up on some of these ergonomic issues.

scotttrinh avatar Feb 20 '24 19:02 scotttrinh

Hi @scotttrinh, thanks. I did not think of this solution. It's neat, but I guess it's not very intuitive. So maybe having helper functions or aliases, (at least in the doc) would be helpful

dotlouis avatar Feb 21 '24 17:02 dotlouis

@dotlouis There is some existing guidance in the "Filtering" section of the client docs here: https://www.edgedb.com/docs/clients/js/select#filtering

Also, there is some similarity here between TypeScript/JavaScript's &&/|| operators and the Array.prototype#every/Array.prototype#some, so hopefully that gives some further intuition here if you're familiar with how those array functions work.

RE: helper functions and aliases

We've tried to keep the query builder syntax as close as possible to GraphQL to make learning one give some intuition about the other, so we've avoided having constructs in the query builder that are sugar for existing patterns. But indeed, the topic being discussed here is exactly that sort of thing: can we introduce some useful functions to make some common patterns more ergonomic.

scotttrinh avatar Feb 21 '24 19:02 scotttrinh

You're right the doc is perfectly clear. I guess the original issue is that I got recommended a e.and() method that did not exist from the AI chat-bot while it should have pointed me to the above solution.

dotlouis avatar Feb 21 '24 19:02 dotlouis