Rewrite documentation for empty filters/predicates #40

acerbusace · 2024-11-25T16:14:27Z

Rewrite documentation for empty filters/predicates, based on the changes of #6 PR.

myronmarston

Left a few suggestions--and I've got one "meta" suggestion: I found it confusing that your branch name and PR title say that this PR updates the readme when it does not in fact make changes to README.md. It would help avoid confusion if "README" is used only to refer to a README.md file.

In this case I'd say that his updates the documentation for anyOf and not.

Although that makes me realize something: instead of narrowly focusing on the documentation of anyOf and not, I think we need to rewrite the documentation of empty filters/predicates entirely. That rewrite will likely say more about anyOf and not since the behavior there is noteworthy but I don't see this task as limited to anyOf and not.

config/site/src/_includes/filtering_predicate_definitions/conjunctions.md

myronmarston · 2024-11-25T18:41:13Z

config/site/src/_includes/filtering_predicate_definitions/conjunctions.md

+
+  > [!WARNING]
+  > When a object is passed, it will be treated as a single object within a list.
+  > For example, `anyOf: {...}` is the same as `anyOf: [{...}]`


This issue is already explained in depth here:

https://block.github.io/elasticgraph/query-api/filtering/conjunctions/#warning-always-pass-a-list

I don't think we need to repeat it here. If you see anything in that section that can be improved, feel free to do so in a follow up PR!

Makes sense, I'll remove the warning from here.

config/site/src/_includes/filtering_predicate_definitions/not.md

myronmarston

Looking good, if we want to stick with the true/false phrasing, but as per my inline comment I wonder if it's clearer to use "matches all documents"/"matches no documents" instead?

Also, I'd like to see this updated as part of this PR:

https://github.com/block/elasticgraph/blob/main/config/site/src/query-api/filtering.md#ignored-filters

myronmarston · 2024-11-27T20:20:18Z

config/site/src/_includes/filtering_predicate_definitions/conjunctions.md

-  Will be ignored when `null` is passed. When an empty list is passed, will
-  cause this part of the filter to match no documents.
+  Will be treated as `true` when `null` or an empty object is passed.
+  When an empty list is passed, will cause this part of the filter to match no documents.


"Will be treated as true" and "cause this part of the filter to match no documents" are opposites but aren't phrased as such, so it's an odd pairing. Maybe we should standardize on one of these pairs:

"Will be treated as true"/"Will be treated as false"

"cause this part of the filter to match all documents"/"cause this part of the filter to match no documents"

Overall, I'm thinking that "matches all documents"/"matches no documents" is clearer than "treated as true"/"treated as false". In the context of a SQL WHERE clause, where you can use a literal true or false, "treated as (true|false)" makes a lot of sense, but we don't let users provide a literal true or false in our GraphQL filter syntax. So maybe it's worth rewording all the docs to speak in terms of "matching all documents" and "matching no documents" rather than true/false.

That's a loosely held opinion, though, and I realize it essentially involves redoing this PR. But if there's consensus that it's clearer, probably worth it.

@BrianSigafoos-SQ I'm curious what you think?

I would prefer to keep it "Will be treated as true"/"Will be treated as false", since we end up using that phrase in other parts of the project (e.g. tests). Though I can be swayed either ways depending on the consensus.

That SGTM. I'd also consider making it more consistent and slightly less wordy.

From this:

Will be treated as `true` when `null` is passed. When an empty list is passed, will cause this part of the filter to match no documents.

To this:

When `null` is passed, matches all documents. When an empty list is passed, this part of the filter matches no documents.

Since, you both like "matches" phrase more, I'll update the PR to use that wording instead. I also like the less wordy approach.

myronmarston · 2024-11-27T20:22:51Z

config/site/src/_includes/filtering_predicate_definitions/conjunctions.md

@@ -6,11 +6,11 @@
  filters that can't be provided on a single filter input because of collisions between key names.
  For example, if you want to provide multiple `anySatisfy: ...` filters, you could do `allOf: [{anySatisfy: ...}, {anySatisfy: ...}]`.

-  Will be ignored when `null` or an empty list is passed.
+  Will be treated as `true` when `null`, an empty object or list is passed.


Do we want to mention the empty object case here? The schema type system doesn't allow empty object here, but it gets coerced from {} into [{}]. I don't really want to encourage users to not pass an object here...

Makes sense, I have removed the mention of empty object.

myronmarston · 2024-11-27T20:23:26Z

config/site/src/_includes/filtering_predicate_definitions/conjunctions.md


 [`anyOf`]({% link query-api/filtering/conjunctions.md %})
 : Matches records where any of the provided sub-filters evaluate to true.
  This works just like an `OR` operator in SQL.

-  Will be ignored when `null` is passed. When an empty list is passed, will
-  cause this part of the filter to match no documents.
+  Will be treated as `true` when `null` or an empty object is passed.


Similar comment here about empty object: there's a gotcha related to that and we don't want to encourage empty objects to be passed so maybe not worth mentioning?

Makes sense, I have removed the mention of empty object.

myronmarston · 2024-11-27T22:34:01Z

config/site/src/query-api/filtering.md


-Filters with a value of `null` or empty object (`{}`) are ignored. The filters in this query are all ignored:
+Filters with a value of `null` or empty object (`{}`) match all documents. Expect for `not` in which case no documents are matched.


Suggested change

Filters with a value of `null` or empty object (`{}`) match all documents. Expect for `not` in which case no documents are matched.

Filters with a value of `null` or empty object (`{}`) match all documents. When negated with `not`, no documents are matched.

myronmarston · 2024-11-27T22:35:54Z

config/site/src/query-api/filtering.md

@@ -17,12 +17,13 @@ As shown here, filters have two basic parts:
  you'll need to provide a nested object matching the field structure.
 * A _filtering predicate_: this specifies a filtering operator to apply at the field path.

-### Ignored Filters
+### Null Or Empty Filters


I think we can just call this "Empty Filters". To me a filter with a value of null is still an empty filter.

And if you change that, maybe rename NullOrEmptyFilters to EmptFilters?

Makes sense, changed to EmptFilters.

myronmarston · 2024-11-27T22:37:53Z

...a_definition/spec/unit/elastic_graph/schema_definition/graphql_schema/built_in_types_spec.rb

-              part of the filter to match no documents. When `null` is passed in the list, will
-              match records where the field value is `null`.
+              When `null` is passed, matches all documents. When an empty list is passed, will
+              cause this part of the filter to match no documents. When `null` is passed in the


/nit Elsewhere you did away with the "will cause" language--can you do that here?

Removed the "will cause" language everywhere.

myronmarston · 2024-11-27T22:38:07Z

...a_definition/spec/unit/elastic_graph/schema_definition/graphql_schema/built_in_types_spec.rb

-              part of the filter to match no documents. When `null` is passed in the list, will
-              match records where the field value is `null`.
+              When `null` is passed, matches all documents. When an empty list is passed, will
+              cause this part of the filter to match no documents. When `null` is passed in the


The "will cause" language could be remove here too.

Removed the "will cause" language everywhere.

myronmarston · 2024-11-27T22:38:32Z

...h-schema_definition/spec/unit/elastic_graph/schema_definition/graphql_schema/filters_spec.rb

-              part of the filter to match no documents. When `null` is passed in the list, will
-              match records where the field value is `null`.
+              When `null` is passed, matches all documents. When an empty list is passed, will
+              cause this part of the filter to match no documents. When `null` is passed in the


The "will cause" language could be removed here too.

(And with that I'll stop commenting on the other spots with it--please clean those up as well!)

Removed the "will cause" language everywhere.

myronmarston

👍 Great work here @acerbusace!

myronmarston · 2024-11-28T17:13:15Z

config/site/examples/music/queries/filtering/EmptyFilters.graphql

@@ -1,4 +1,4 @@
-query IgnoredFilters {
+query NullOrEmptyFilters {


Suggested change

query NullOrEmptyFilters {

query EmptyFilters {

...to match the filename.

Didn't realize you had auto-merge on (which is fine), so this was merged when I approved. I've got a followup PR to fix this here: #41 if you want to review it.

Approved, thanks!

acerbusace requested review from myronmarston and BrianSigafoos-SQ as code owners November 25, 2024 16:14

myronmarston requested changes Nov 25, 2024

View reviewed changes

acerbusace changed the title ~~Update readme for anyOf and not~~ Rewrite documentation for empty filters/predicates Nov 27, 2024

acerbusace force-pushed the block/alexp/readmeUpdates branch 6 times, most recently from 87e0570 to 9a0f067 Compare November 27, 2024 18:41

myronmarston requested changes Nov 27, 2024

View reviewed changes

acerbusace force-pushed the block/alexp/readmeUpdates branch 4 times, most recently from 2b23c8c to 210ff2f Compare November 27, 2024 22:15

acerbusace requested a review from myronmarston November 27, 2024 22:17

myronmarston reviewed Nov 27, 2024

View reviewed changes

Update readme for anyOf and not

5ab6a57

acerbusace force-pushed the block/alexp/readmeUpdates branch from 210ff2f to 5ab6a57 Compare November 28, 2024 15:10

acerbusace requested a review from myronmarston November 28, 2024 15:33

acerbusace enabled auto-merge (squash) November 28, 2024 15:33

myronmarston approved these changes Nov 28, 2024

View reviewed changes

acerbusace merged commit c2d3da7 into main Nov 28, 2024
10 checks passed

acerbusace deleted the block/alexp/readmeUpdates branch November 28, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite documentation for empty filters/predicates #40

Rewrite documentation for empty filters/predicates #40

acerbusace commented Nov 25, 2024 •

edited

Loading

myronmarston left a comment

myronmarston Nov 25, 2024

acerbusace Nov 27, 2024

myronmarston left a comment

myronmarston Nov 27, 2024

acerbusace Nov 27, 2024

BrianSigafoos-SQ Nov 27, 2024

acerbusace Nov 27, 2024

myronmarston Nov 27, 2024

acerbusace Nov 27, 2024

myronmarston Nov 27, 2024

acerbusace Nov 27, 2024

myronmarston Nov 27, 2024

acerbusace Nov 28, 2024

myronmarston Nov 27, 2024

acerbusace Nov 28, 2024

myronmarston Nov 27, 2024

acerbusace Nov 28, 2024 •

edited

Loading

myronmarston Nov 27, 2024

acerbusace Nov 28, 2024 •

edited

Loading

myronmarston Nov 27, 2024

acerbusace Nov 28, 2024

myronmarston left a comment

myronmarston Nov 28, 2024

myronmarston Nov 28, 2024

acerbusace Nov 28, 2024


		Filters with a value of `null` or empty object (`{}`) are ignored. The filters in this query are all ignored:
		Filters with a value of `null` or empty object (`{}`) match all documents. Expect for `not` in which case no documents are matched.

		@@ -1,4 +1,4 @@
		query IgnoredFilters {
		query NullOrEmptyFilters {

Rewrite documentation for empty filters/predicates #40

Rewrite documentation for empty filters/predicates #40

Conversation

acerbusace commented Nov 25, 2024 • edited Loading

myronmarston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerbusace Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerbusace Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerbusace commented Nov 25, 2024 •

edited

Loading

acerbusace Nov 28, 2024 •

edited

Loading

acerbusace Nov 28, 2024 •

edited

Loading