lukesneeringer · 2020-03-24T20:05:03Z

This issue is intended for public comment, and should remain open through April 24, 2020.

odsod · 2020-03-30T14:27:46Z

@lukesneeringer Just curious - do you know if there are any support libraries that can be used for implementing filter parsers, or do we use the EBNF grammar to roll our own?

Also, I think this paragraph sends mixed messages:

APIs may provide filtering to users on List methods (or similar methods to query a collection, such as Search). If they choose to do so, they must follow the common specification for filters discussed here. The syntax is formally defined in the EBNF grammar.

Note: List Filters have fuzzy matching characteristics with support for result ranking and scoring. For developers interested in deterministic evaluation of list filters, see CEL.

Should it say "should follow the common specification for filters discussed here" since CEL filters are brought up as an alternative?

lukesneeringer · 2020-04-02T19:11:20Z

Just curious - do you know if there are any support libraries that can be used for implementing filter parsers, or do we use the EBNF grammar to roll our own?

I believe we have some internal libraries. @TristonianJones would know for sure what they are.

Should it say "should follow the common specification for filters discussed here" since CEL filters are brought up as an alternative?

Good point. Fixing.

0x4b · 2020-04-02T19:12:36Z

It would be helpful to have guidance on whether filtering should all be done in a single request field.

For example, if one were to add filtering to ListBooks, and we want to allow filtering across publishers (assuming aip.dev/159 is in effect) and expect it to be a commonly filtered attribute of books:

May there be a separate ListBooks.publisher_filter field (assuming it and any other filter field follow the common specification)?

May a commonly filtered attribute be done with a repeated field listing values to match, assuming that general filtering that follows the guidance also exists?

lukesneeringer · 2020-04-02T19:13:38Z

It would be helpful to have guidance on whether filtering should all be done in a single request field.

Single field. The way to do filtering on related objects would be something akin to publisher.title = "foo" or similar.

I will add something for this.

bMacSwigg · 2020-04-02T21:26:29Z

Just curious - do you know if there are any support libraries that can be used for implementing filter parsers, or do we use the EBNF grammar to roll our own?

go/filtering is a very similar design, and is implemented internally at cs/cloud/api/filters with a Java library at java/com/google/cloud/api/filter. I'm not sure if it's 100% compliant with the AIP, but if not I think it would be good for them to converge.

bMacSwigg · 2020-04-02T21:29:39Z

bMacSwigg commented

Apr 2, 2020

The AIP notes:

Literals appearing alone are matched anywhere it may appear in an object’s field values.

Is this a requirement? For certain fields, this may not be feasible. In particular, we have some fields in our APIs that are exposed as email addresses externally but converted to Gaia IDs in storage. If a user passes a literal, it would be difficult-to-impossible (and provide questionable value) to try to match that literal against every email address represented by the Gaia IDs we have stored.

TristonianJones · 2020-04-02T21:38:32Z

@0x4b I've implemented a slightly more opinionated version of the filter EBNF with ANTLR before for grafeas.io. I've heard good things about peg.js as well.

@bMacSwigg I would recommend continuing this conversation over internal message boards rather than in public. What you see is compatible with the intent of the original filter design, though there have been some features developed for specific use cases which aren't discussed here

lukesneeringer · 2020-04-24T20:58:46Z

Note: We are bumping approval of this one to May to allow for certain questions that have come up to be resolved. You should be seeing (mostly minor) revisions to this on this PR soon.

Some of this was discussed over internal channels.

Co-authored-by: bMacSwigg <brian.mcswiggen@gmail.com>

googlebot · 2020-05-05T22:14:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-05T22:15:16Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

lukesneeringer · 2020-05-05T22:15:29Z

@bMacSwigg works here, gidget.

garyxue · 2020-05-06T00:12:42Z

I feel that this AIP intrudes into the UX design for a product, instead of narrowly focusing on API design. I read the statement "...therefore it is prudent to use a string field with a structured syntax accessible to a non-technical audience" as implying that this filter expression SHOULD be user-facing. However applications should be able to tailor their UI experience (including how filters are created) based on their own unique set of requirements. Some may need more sophisticated expression syntax for a tech-oriented product (think Common Expression Language. Consumer-facing apps (think your typical shopping portal) probably will limit filtering building to UI dialogs only. Dictating an expression syntax as part of the API will mandate a parser implementation (likely in both FE and BE) and some expensive translation between what the UI presents and what the API takes.

And if you do mandate a filter expression syntax, wouldn't it be prudent wait until public domain parsers are available for most languages before making this a requirement?

vladbarosan · 2020-05-10T22:48:09Z

-Additionally, when comparing strings for equality, the `*` character denotes
-wildcard syntax; for example, `a = "*.foo"` is true if `a` _ends with_ ".foo".
+Additionally, when comparing strings for equality, services **should** support
+wildcards using the `*` character; for example, `a = "*.foo"` is true if `a`


is this intended as a suffix/prefix match only or general wildcard support ? comments on the list filtering design doc seem to suggest only allowing * as a suffix ( i.e. allow only prefix matching ).

It seems fine to do more, but we probably only want to require prefix/suffix because of subtleties around values with * in them.

googlebot · 2020-05-20T21:11:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

lukesneeringer · 2020-05-20T21:36:13Z

And if you do mandate a filter expression syntax, wouldn't it be prudent wait until public domain parsers are available for most languages before making this a requirement?

This is a good point, @garyxue. We have a couple of parsers now, but it would be useful to see if we could open source them.

jskeet

I've only reviewed the changes here, and this is a generally subtle AIP which I know there has been a lot of iteration on. I haven't carefully reviewed everything else, but I trust others have for previous PRs.

jskeet · 2020-05-27T07:36:58Z

-`!=`, `<`, `>`, `<=`, and `>=`.
+Filtering implementations **should** provide the binary comparison operators
+`=`, `!=`, `<`, `>`, `<=`, and `>=` for string, numeric, timestamp, and
+duration fields (but **should not** provide them for booleans or enums).


Enums is an interesting one. Some enums are naturally ordered, others aren't. I'm okay with having this here for the moment, but I suspect it's something we may want to tweak over time.

jskeet · 2020-05-27T07:39:45Z

 | `a.b > 42`      | True if `a` has a numeric `b` field that is above 42. |
 | `a.b.c = "foo"` | True if `a.b` has a string `c` field that is "foo".   |

+Traversal **must** be written using the field names from the resource. If a


Field names as specified in the original proto, presumably.

For field masks, the JSON representation performs camel casing... presumably there's nothing equivalent here, at least not yet? (I can see how that state of affairs could lead to some odd query strings - if you specify both a filter and a field mask in a query string, the two could specify the same field in different ways.)

I actually think our filtering implementations accept both?

They're supposed to accept both forms

googlebot · 2020-05-27T16:01:07Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

lukesneeringer · 2020-05-27T16:01:31Z

They all still work here.

sam-utila · 2024-01-14T14:49:13Z

Traversal must be written using the field names from the resource.

What happens when I need to filter on a related object, is that something not supported by APIs that follow AIP?

message Book {
  string name = 1; // resource name
  string author = 1; // resource name of the author
  bool old = 2;
}

message Author {
  string name = 1;
  string display_name = 2;
  bool famous = 3;
}

How should I do: ListBooks with filter of author.display_name=*ab* AND old

Since author.display_name does not really exist on the Book message.

According to the documentation I need to have special author_display_name()==? function supported by the filter.
And that means also author_famous()==? function for the second field if I also want to filter by that one.

Instead of allowing author.display_name and author.famous.

This is easy in 1:1 cases.

If suddenly a book has multiple authors:

message Book {
  string name = 1; // resource name
  repeated string author = 1; // resource name of the author
  bool old = 2;
}

Then we suddenly have none, some and all filtering which is less straight forward.
i.e. List all books where all authors are famous, some authors are famous and none of the authors are famous - maybe a function here is more straightforward.

However, I didn't see any API in googleapis what I can take an inspiration from

[AIP-160] Approve the filtering AIP.

8ed1242

lukesneeringer added this to the April 2020 Newsletter milestone Mar 24, 2020

lukesneeringer requested a review from a team March 24, 2020 20:05

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2020

Incorporate @odsod feedback.

bf29eaa

Incorporate @0x4b feedback.

43e3c15

lukesneeringer modified the milestones: April 2020 Newsletter, May 2020 Newsletter Apr 24, 2020

Incorporate @bMacSwigg, @TristonianJones feedback.

f89a855

Some of this was discussed over internal channels.

bMacSwigg reviewed May 5, 2020

View reviewed changes

Comment thread aip/0160.md Outdated

Incorporate @bMacSwigg feedback.

892ac6d

Co-authored-by: bMacSwigg <brian.mcswiggen@gmail.com>

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 5, 2020

lukesneeringer added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 5, 2020

vladbarosan reviewed May 10, 2020

View reviewed changes

Discuss traversal to undefined keys.

3c5023b

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 20, 2020

jskeet approved these changes May 27, 2020

View reviewed changes

lukesneeringer added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 27, 2020

lukesneeringer merged commit 6321f4d into master May 29, 2020

lukesneeringer deleted the aip-160-approval branch May 29, 2020 15:42

Navigation Menu

Uh oh!

[AIP-160] Approve the filtering AIP. - #473