Change anyOf behaviour to evaluate to true for empty predicates #6

acerbusace · 2024-10-29T16:00:56Z

Context

Currently empty predicates are ignored. Within an AND context (the normal context), that works correctly. Within an OR context (under an anyOf) it does not work correctly: empty predicates should essentially be treated as true:

predicate AND emptyPredicate should be like predicate AND true which reduces to predicate (and thus is equivalent to ignoring the empty predicate).
predicate OR emptyPredicate should be like predicate OR true which just reduces to true

Notably, filter: A and filter: {anyOf: [A]} should behave the same but right now they do not when A is an empty predicate – filter: emptyPredicate returns all results whereas filter: {anyOf: [emptyPredicate]} returns no results.

Change

This PR as mentioned above, looks to change filter: {field: nil} and filter: {field: {}} to evaluate to true. This in turns changes how anyOf behaves to be more like an OR block, as shown below:

filter: {anyOf: [emptyPredicate]} will evaluate to true, instead of false
filter: {anyOf: [emptyPredicate, {name: {equalToAnyOf: ["test"]}}]} will evaluate to true, instead of false

At the same time the behaviour of (filter: {anyOf: []}) matching no documents is still preserved.

Because of the way emptyPredicates are treated now, the following changes occured for how not behaves:

filter: {not: {field: nil}} will evaluate to false, instead of being ignored

myronmarston

💯 great work on this @acerbusace! I'm glad the implementation turned out to be pretty simple.

Besides my inline suggestions, I have a few more related to some additional test coverage.

It's essential that our shard routing logic and index selection logic work correctly with all filtering operators. I took a look at the underlying implementation and it looks like it was already working correctly here since it treats a nil predicate as matching all values but the specs don't cover the any_of: [{field: nil}] case. To that end can you add some specs in these spots for that?

elasticgraph/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb

Lines 367 to 371 in 0e5352a

    
           it "excludes no indices when we have an `any_of: []` filter because that will match all results" do 
        
             parts = search_index_expression_parts_for({"any_of" => []}) 
        
             expect(parts).to target_all_widget_indices 
        
           end

elasticgraph/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb

Lines 256 to 260 in 0e5352a

    
           it "searches all shards when we have an `any_of: []` filter because that will match all results" do 
        
             expect(shard_routing_for(["name"], { 
        
               "any_of" => [] 
        
             })).to search_all_shards 
        
           end

I'm 99% sure that this works correctly when combined with not (e.g. not: {any_of: [{field: null}]}) but we don't have any tests to prove it. Can you add some? Would be great to add coverage at all 3 levels (unit, integration, and acceptance) if that's easy to do (which I suspect it is).

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

myronmarston · 2024-10-29T17:30:44Z

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

One additional, post-review suggestion: this files talks about "ignoring" nil/empty predicates in 3 places:

62: # `nil` filter predicates should be ignored, so we can safely `compact` them out. 69: # This is an "empty" filter predicate and we can ignore it. 339: # To ignore "empty" filter predicates we need to return `nil` here.

The language of "ignoring" allowed this bug in the first place because "ignore" sounds like "pretend it isn't there", but we really need to treat it as true instead.

Can you update those comments to more clearly explain? e.g. the first one could be:

# `nil` filter predicates should be treated as `true`, so we can safely `compact` them out.

Can we apply this more broadly across the code base? Here's a grep that returns candidates for this treatment:

 ~/Development/elasticgraph/ [block/alexp/anyofchanges] ag ignore elasticgraph-graphql -G ".rb" elasticgraph-graphql/sig/elastic_graph/graphql/datastore_query.rbs 3: # Note: this is a partial signature definition (`query.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema/type.rbs 4: # Note: this is a partial signature definition (`type.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema/field.rbs 4: # Note: this is a partial signature definition (`field.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema.rbs 3: # Note: this is a partial signature definition (`schema.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/datastore_response/search_response.rbs 4: # Note: this is a partial signature definition (`search_response.rb` is ignored in `Steepfile`) elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb 256: it "ignores an empty filter" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_search_router_spec.rb 101: it "ignores queries that have no `monotonic_clock_deadline` when picking the overall timeout" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb 241: it "ignores unknown variables" do 281: it "does not ignore `null` values on unknown arguments in the query itself" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb 536: it "ignores `filter: null`" do 550: it "ignores a `field: null` filter since it will get pruned" do 582: it "ignores a `field: {predicate: null}` filter since it will get pruned" do 617: it "ignores a `null` filter since it will get pruned" do 652: it "ignores a `parent_field: {child_field: null}` filter since it will get pruned" do 687: it "ignores a `parent_field: {child_field: {predicate: null}}` filter since it will get pruned" do 935: it "includes the incomplete doc exclusion filter when there are no sub-clauses, because the filter is ignored" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/requested_fields_spec.rb 326: it "ignores relay connection sub-fields that are not directly under `edges.node` (e.g. `page_info`)" do 696: it "ignores built-in introspection fields as they never exist in the datastore" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb 181: it "ignores empty filters" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb 113: it "ignores filter operators it does not understand" do 115: # if it did we should just ignore it. 173: it "ignores `nil` when `equal_to_any_of` includes `nil` with other timestamps" do 328: it "ignores a `nil` value for a `#{operator}` filter" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb 33: it "ignores unknown filtering operators and logs a warning" do 41: it "ignores malformed filters and logs a warning" do 381: it "is ignored when `null` is passed" do 387: it "is ignored when `[]` is passed" do 789: it "ignores `count` filter predicates that have a `nil` or `{}` value" do 1280: it "is ignored when set to nil" do 1287: it "is ignored when set to nil when alongside other filters" do 1303: it "is ignored when the inner filter is also ignored" do 1507: # RSpec's differ doesn't ignore whitespace, but we want our diffs here to do so to make it easier to read the output. 1517: `git diff --no-index #{actual_file.path} #{expected_file.path} --ignore-all-space #{" --color" unless ENV["CI"]}` elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sorting_spec.rb 28: it "ignores duplicate sort fields, preferring whichever direction comes first" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb 61: it "ignores `nil` among other values in `equal_to_any_of` filter for a single `route_with_field_paths` field" do 77: it "ignores inequality operators on a single `route_with_field_paths` field when that field also has an exact equality operator" do 78: # the fact that we are filtering `> abc` can be ignored because we are only looking for `def` based on `equal_to_any_of`. 91: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in one hash)" do 98: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in an array of hashes)" do 165: it "searches all shards when the query filters with `equal_to_any_of: nil` on a single `route_with_field_paths` field because our current filter logic ignores that filter and we must search all shards" do 171: it "searches all shards when the query filters with `equal_to_any_on` on a single `route_with_field_paths` field only contains ignored routing values" do 174: {"name" => {"equal_to_any_of" => ["ignored_value"]}}, 175: ignored_routing_values: ["ignored_value"] 179: it "searches all shards when the query filters with `equal_to_any_on` on a single `route_with_field_paths` field contains both an ignored routing value and non-ignored routing values" do 182: {"name" => {"equal_to_any_of" => ["ignored_value", "not_ignored_value"]}}, 183: ignored_routing_values: ["ignored_value"] 351: it "ignores inequality operators on a single `route_with_field_paths` field when that field also has an exact equality operator" do 352: # the fact that we are filtering `!(> xyz)` can be ignored because we are only looking for `def` based on `equal_to_any_of`. 365: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in one hash)" do 372: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in an array of hashes)" do 491: def shard_routing_for(route_with_field_paths, filter_or_filters, ignored_routing_values: [], aggregations: nil) 498: search_index_definitions = search_index_definitions_for(route_with_field_paths, ignored_routing_values) 523: def search_index_definitions_for(route_with_field_paths, ignored_routing_values) 525: ["index#{i}", config_index_def_of(ignore_routing_values: ignored_routing_values)] elasticgraph-graphql/spec/unit/elastic_graph/graphql/http_endpoint_spec.rb 157: it "ignores `operationName` if set to an empty string" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb 113: it "ignores it, allowing the entire array to be returned because we've already paid the cost of fetching the entire array from the datastore, and limiting the page size here doesn't really help us" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/query_adapter_spec.rb 136: it "ignores the pagination arguments since the aggregation adapter handles them for aggregations" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb 196: it "ignores empty filters" do 993: it "ignores date histogram buckets that have no documents in them" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb 28: # Verify that empty strings (and other types like numbers) that don't match anything are ignored. 286: it "correctly ignores nil filters under `any_satisfy`" do 1176: it "is ignored when set to nil" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/resolvers/nested_relationships_spec.rb 424: # The datastore's `search` using `ignore_unavailable: true`, the same thing is not supported for `msearch`, elasticgraph-graphql/spec/acceptance/search_spec.rb 33: workspace_id: "ignored_workspace_2", 113: filter: {id: {equal_to_any_of: [widget1.fetch(:id), widget2.fetch(:id), "", " ", "\n"]}}, # empty strings should be ignored. 184: # Verify that we can filter on `DateTime` fields (and that `nil` DateTime filter values are ignored) 232: # Verify that we can filter on `Date` fields (and that `nil` Date filter values are ignored) 315: filter: {name: {equal_to_any_of: [nil, "thing2", "", " ", "\n"]}}, # empty strings should be ignored., 353: # The negation of an ignored filter is an ignored filter: `{not: {equal_to_any_of: nil}}` 354: # evaluates to {not: {}} which will be ignored, therefore the filter will match all documents. 366: # (The extra sort should be effectively ignored). 381: # Test that we can query for widgets with ignored routing values 383: filter: {"workspace_id" => {"equal_to_any_of" => ["workspace_1", "ignored_workspace_2"]}}, 391: filter: {"workspace_id" => {"not" => {"equal_to_any_of" => ["workspace_1", "ignored_workspace_2"]}}}, elasticgraph-graphql/spec/acceptance/aggregations_spec.rb 1095: "DAY_FOR_HOUR" => {case_correctly("as_date_time") => "2019-06-01T12:00:00.000Z"}, # When grouping by hour but offset by 4 days, the offset is effectively ignored. 1144: "DAY_FOR_HOUR" => "2019-06-01T12:00:00.000Z", # When grouping by hour but offset by 4 days, the offset is effectively ignored. elasticgraph-graphql/lib/elastic_graph/graphql/query_executor.rb 51: # schema if we ignore null-valued fields rather than potentially returning an error elasticgraph-graphql/lib/elastic_graph/graphql/datastore_query.rb 215: elsif contains_ignored_values_for_routing?(routing_values) 307: def contains_ignored_values_for_routing?(routing_values) 308: ignored_values_for_routing.intersect?(routing_values.to_set) if routing_values 311: def ignored_values_for_routing 312: @ignored_values_for_routing ||= search_index_definitions.flat_map { |i| i.ignored_values_for_routing.to_a }.to_set elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/requested_fields.rb 53: # can ignore its foreign key; but when we are determining requested fields for a parent type, elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb 86: # Any filter with a `nil` value is effectively ignored by our filtering logic, so we need elasticgraph-graphql/lib/elastic_graph/graphql/http_endpoint.rb 130: # Ignore an empty string operationName. elasticgraph-graphql/lib/elastic_graph/graphql/datastore_search_router.rb 59: # [4] https://discuss.elastic.co/t/timeouts-ignored-in-multisearch/23673

Can you review those and update any that use "ignore" to describe how an empty filter predicate is treated?

Went through most of the files and changed the wording. Still need to do the following:

shard_routing_spec.rb

filter_value_set_extractor

acerbusace · 2024-11-01T20:54:18Z

This PR, forgets to take into account that {"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 0}}]} should evaluate to true. Instead right now, it evaluates to false. To fix, this it will require more restructuring then what this PR does. Hence, created a new PR that changes all the behaviours as mentioned in this PR and fixes the problem I mentioned just above.

acerbusace · 2024-11-06T15:53:33Z

This PR, forgets to take into account that {"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 0}}]} should evaluate to true. Instead right now, it evaluates to false. To fix, this it will require more restructuring then what this PR does. Hence, created a new PR that changes all the behaviours as mentioned in this PR and fixes the problem I mentioned just above.

Nvm, the above case can be supported with a simple size check!

myronmarston

I like how you found a way to fix anyOf without needing to do the larger refactoring we had discussed!

That said, I think this may still have bugs with not and anySatisfy. I think it's important that we holistically solve all these cases and release the fixes in a single release, so that the disruption is only felt by our users once. So, before I'm ready to merge this I'd like us to figure out if we can fix the the not and anySatisfy cases in a similar way (and cover them with tests). Note, however, that the improvements for those cases could be done in stacked PRs if you want to keep this one small and focused on anyOf.

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb

...raph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

myronmarston · 2024-11-07T00:42:45Z

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

Can we apply this more broadly across the code base? Here's a grep that returns candidates for this treatment:

 ~/Development/elasticgraph/ [block/alexp/anyofchanges] ag ignore elasticgraph-graphql -G ".rb" elasticgraph-graphql/sig/elastic_graph/graphql/datastore_query.rbs 3: # Note: this is a partial signature definition (`query.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema/type.rbs 4: # Note: this is a partial signature definition (`type.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema/field.rbs 4: # Note: this is a partial signature definition (`field.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/schema.rbs 3: # Note: this is a partial signature definition (`schema.rb` is ignored in `Steepfile`) elasticgraph-graphql/sig/elastic_graph/graphql/datastore_response/search_response.rbs 4: # Note: this is a partial signature definition (`search_response.rb` is ignored in `Steepfile`) elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb 256: it "ignores an empty filter" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_search_router_spec.rb 101: it "ignores queries that have no `monotonic_clock_deadline` when picking the overall timeout" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb 241: it "ignores unknown variables" do 281: it "does not ignore `null` values on unknown arguments in the query itself" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb 536: it "ignores `filter: null`" do 550: it "ignores a `field: null` filter since it will get pruned" do 582: it "ignores a `field: {predicate: null}` filter since it will get pruned" do 617: it "ignores a `null` filter since it will get pruned" do 652: it "ignores a `parent_field: {child_field: null}` filter since it will get pruned" do 687: it "ignores a `parent_field: {child_field: {predicate: null}}` filter since it will get pruned" do 935: it "includes the incomplete doc exclusion filter when there are no sub-clauses, because the filter is ignored" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/requested_fields_spec.rb 326: it "ignores relay connection sub-fields that are not directly under `edges.node` (e.g. `page_info`)" do 696: it "ignores built-in introspection fields as they never exist in the datastore" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb 181: it "ignores empty filters" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb 113: it "ignores filter operators it does not understand" do 115: # if it did we should just ignore it. 173: it "ignores `nil` when `equal_to_any_of` includes `nil` with other timestamps" do 328: it "ignores a `nil` value for a `#{operator}` filter" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb 33: it "ignores unknown filtering operators and logs a warning" do 41: it "ignores malformed filters and logs a warning" do 381: it "is ignored when `null` is passed" do 387: it "is ignored when `[]` is passed" do 789: it "ignores `count` filter predicates that have a `nil` or `{}` value" do 1280: it "is ignored when set to nil" do 1287: it "is ignored when set to nil when alongside other filters" do 1303: it "is ignored when the inner filter is also ignored" do 1507: # RSpec's differ doesn't ignore whitespace, but we want our diffs here to do so to make it easier to read the output. 1517: `git diff --no-index #{actual_file.path} #{expected_file.path} --ignore-all-space #{" --color" unless ENV["CI"]}` elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sorting_spec.rb 28: it "ignores duplicate sort fields, preferring whichever direction comes first" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb 61: it "ignores `nil` among other values in `equal_to_any_of` filter for a single `route_with_field_paths` field" do 77: it "ignores inequality operators on a single `route_with_field_paths` field when that field also has an exact equality operator" do 78: # the fact that we are filtering `> abc` can be ignored because we are only looking for `def` based on `equal_to_any_of`. 91: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in one hash)" do 98: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in an array of hashes)" do 165: it "searches all shards when the query filters with `equal_to_any_of: nil` on a single `route_with_field_paths` field because our current filter logic ignores that filter and we must search all shards" do 171: it "searches all shards when the query filters with `equal_to_any_on` on a single `route_with_field_paths` field only contains ignored routing values" do 174: {"name" => {"equal_to_any_of" => ["ignored_value"]}}, 175: ignored_routing_values: ["ignored_value"] 179: it "searches all shards when the query filters with `equal_to_any_on` on a single `route_with_field_paths` field contains both an ignored routing value and non-ignored routing values" do 182: {"name" => {"equal_to_any_of" => ["ignored_value", "not_ignored_value"]}}, 183: ignored_routing_values: ["ignored_value"] 351: it "ignores inequality operators on a single `route_with_field_paths` field when that field also has an exact equality operator" do 352: # the fact that we are filtering `!(> xyz)` can be ignored because we are only looking for `def` based on `equal_to_any_of`. 365: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in one hash)" do 372: it "ignores filters on other fields so long as they are not in an `any_of` clause (for multiple filters in an array of hashes)" do 491: def shard_routing_for(route_with_field_paths, filter_or_filters, ignored_routing_values: [], aggregations: nil) 498: search_index_definitions = search_index_definitions_for(route_with_field_paths, ignored_routing_values) 523: def search_index_definitions_for(route_with_field_paths, ignored_routing_values) 525: ["index#{i}", config_index_def_of(ignore_routing_values: ignored_routing_values)] elasticgraph-graphql/spec/unit/elastic_graph/graphql/http_endpoint_spec.rb 157: it "ignores `operationName` if set to an empty string" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb 113: it "ignores it, allowing the entire array to be returned because we've already paid the cost of fetching the entire array from the datastore, and limiting the page size here doesn't really help us" do elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/query_adapter_spec.rb 136: it "ignores the pagination arguments since the aggregation adapter handles them for aggregations" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb 196: it "ignores empty filters" do 993: it "ignores date histogram buckets that have no documents in them" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb 28: # Verify that empty strings (and other types like numbers) that don't match anything are ignored. 286: it "correctly ignores nil filters under `any_satisfy`" do 1176: it "is ignored when set to nil" do elasticgraph-graphql/spec/integration/elastic_graph/graphql/resolvers/nested_relationships_spec.rb 424: # The datastore's `search` using `ignore_unavailable: true`, the same thing is not supported for `msearch`, elasticgraph-graphql/spec/acceptance/search_spec.rb 33: workspace_id: "ignored_workspace_2", 113: filter: {id: {equal_to_any_of: [widget1.fetch(:id), widget2.fetch(:id), "", " ", "\n"]}}, # empty strings should be ignored. 184: # Verify that we can filter on `DateTime` fields (and that `nil` DateTime filter values are ignored) 232: # Verify that we can filter on `Date` fields (and that `nil` Date filter values are ignored) 315: filter: {name: {equal_to_any_of: [nil, "thing2", "", " ", "\n"]}}, # empty strings should be ignored., 353: # The negation of an ignored filter is an ignored filter: `{not: {equal_to_any_of: nil}}` 354: # evaluates to {not: {}} which will be ignored, therefore the filter will match all documents. 366: # (The extra sort should be effectively ignored). 381: # Test that we can query for widgets with ignored routing values 383: filter: {"workspace_id" => {"equal_to_any_of" => ["workspace_1", "ignored_workspace_2"]}}, 391: filter: {"workspace_id" => {"not" => {"equal_to_any_of" => ["workspace_1", "ignored_workspace_2"]}}}, elasticgraph-graphql/spec/acceptance/aggregations_spec.rb 1095: "DAY_FOR_HOUR" => {case_correctly("as_date_time") => "2019-06-01T12:00:00.000Z"}, # When grouping by hour but offset by 4 days, the offset is effectively ignored. 1144: "DAY_FOR_HOUR" => "2019-06-01T12:00:00.000Z", # When grouping by hour but offset by 4 days, the offset is effectively ignored. elasticgraph-graphql/lib/elastic_graph/graphql/query_executor.rb 51: # schema if we ignore null-valued fields rather than potentially returning an error elasticgraph-graphql/lib/elastic_graph/graphql/datastore_query.rb 215: elsif contains_ignored_values_for_routing?(routing_values) 307: def contains_ignored_values_for_routing?(routing_values) 308: ignored_values_for_routing.intersect?(routing_values.to_set) if routing_values 311: def ignored_values_for_routing 312: @ignored_values_for_routing ||= search_index_definitions.flat_map { |i| i.ignored_values_for_routing.to_a }.to_set elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/requested_fields.rb 53: # can ignore its foreign key; but when we are determining requested fields for a parent type, elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb 86: # Any filter with a `nil` value is effectively ignored by our filtering logic, so we need elasticgraph-graphql/lib/elastic_graph/graphql/http_endpoint.rb 130: # Ignore an empty string operationName. elasticgraph-graphql/lib/elastic_graph/graphql/datastore_search_router.rb 59: # [4] https://discuss.elastic.co/t/timeouts-ignored-in-multisearch/23673

Can you review those and update any that use "ignore" to describe how an empty filter predicate is treated?

myronmarston

Almost ready to merge!

elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

elasticgraph-graphql/spec/acceptance/search_spec.rb

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb

myronmarston

👍 Great work @acerbusace! There's a lot of hidden subtley here.

I left one suggestion on a comment that's confusing. Beyond that I wanted to comment on this:

Went through most of the files and changed the wording. Still need to do the following:

shard_routing_spec.rb

filter_value_set_extractor

Deferring this to the PR where you update the shard routing/index selection logic makes sense--but let's be sure to remember to update the confusing uses of "ignore" or "prune" in those places.

Finally, can you squash as part of merging this?

myronmarston · 2024-11-11T20:11:57Z

elasticgraph-graphql/spec/acceptance/search_spec.rb

+
+        expect(widgets).to eq []
+
+        # Test `not: {any_of: [emptyPredicate && nonEmptyPredicate]}` results in no matching documents


I brought this up in #6 (comment) but any_of: [emptyPredicate && nonEmptyPredicate] is quite confusing because any_of is an OR but && commonly means AND.

Can this be any_of: [emptyPredicate, nonEmptyPredicate]?

Thanks, must have forgotten to change from && to ,

acerbusace · 2024-11-11T21:17:30Z

👍 Great work @acerbusace! There's a lot of hidden subtley here.

I left one suggestion on a comment that's confusing. Beyond that I wanted to comment on this:

Went through most of the files and changed the wording. Still need to do the following:

shard_routing_spec.rb

filter_value_set_extractor

Deferring this to the PR where you update the shard routing/index selection logic makes sense--but let's be sure to remember to update the confusing uses of "ignore" or "prune" in those places.

Finally, can you squash as part of merging this?

Ended up changing filter_value_set_extractor, where it mentioned ignore. I'll leave shard_routing_spec for a later PR (add also added TODOs to remember).

acerbusace requested a review from BrianSigafoos-SQ as a code owner October 29, 2024 16:00

acerbusace force-pushed the block/alexp/anyofchanges branch 2 times, most recently from b935acd to 2e0a575 Compare October 29, 2024 16:09

myronmarston requested changes Oct 29, 2024

View reviewed changes

myronmarston reviewed Oct 29, 2024

View reviewed changes

acerbusace force-pushed the block/alexp/anyofchanges branch 3 times, most recently from d7e3af1 to b321908 Compare November 6, 2024 15:47

myronmarston requested changes Nov 7, 2024

View reviewed changes

acerbusace force-pushed the block/alexp/anyofchanges branch 2 times, most recently from 2e7a553 to 4ef723f Compare November 8, 2024 16:27

myronmarston reviewed Nov 9, 2024

View reviewed changes

acerbusace force-pushed the block/alexp/anyofchanges branch 4 times, most recently from b31249d to 55320ce Compare November 11, 2024 16:58

acerbusace requested a review from myronmarston November 11, 2024 16:59

acerbusace force-pushed the block/alexp/anyofchanges branch from 55320ce to eb96123 Compare November 11, 2024 17:21

myronmarston approved these changes Nov 11, 2024

View reviewed changes

acerbusace force-pushed the block/alexp/anyofchanges branch from eb96123 to 8ec54f5 Compare November 11, 2024 21:03

Change anyOf behaviour to evaluate to true for empty predicates

817d91b

acerbusace force-pushed the block/alexp/anyofchanges branch from 8ec54f5 to 817d91b Compare November 11, 2024 21:13

acerbusace merged commit 91dbffd into main Nov 11, 2024
10 checks passed

acerbusace deleted the block/alexp/anyofchanges branch November 11, 2024 21:32

This was referenced Nov 21, 2024

Treat anyOf: [] as matching no routing shards #39

Merged

Rewrite documentation for empty filters/predicates #40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change anyOf behaviour to evaluate to true for empty predicates #6

Change anyOf behaviour to evaluate to true for empty predicates #6

acerbusace commented Oct 29, 2024 •

edited

Loading

myronmarston left a comment

myronmarston Oct 29, 2024 •

edited

Loading

acerbusace Nov 6, 2024

myronmarston Nov 7, 2024

acerbusace Nov 8, 2024

acerbusace commented Nov 1, 2024

acerbusace commented Nov 6, 2024

myronmarston left a comment

myronmarston Nov 7, 2024

myronmarston left a comment

myronmarston left a comment

myronmarston Nov 11, 2024

acerbusace Nov 11, 2024

acerbusace commented Nov 11, 2024

	it "excludes no indices when we have an `any_of: []` filter because that will match all results" do
	parts = search_index_expression_parts_for({"any_of" => []})

	expect(parts).to target_all_widget_indices
	end

	it "searches all shards when we have an `any_of: []` filter because that will match all results" do
	expect(shard_routing_for(["name"], {
	"any_of" => []
	})).to search_all_shards
	end


		expect(widgets).to eq []

		# Test `not: {any_of: [emptyPredicate && nonEmptyPredicate]}` results in no matching documents

Change anyOf behaviour to evaluate to true for empty predicates #6

Change anyOf behaviour to evaluate to true for empty predicates #6

Conversation

acerbusace commented Oct 29, 2024 • edited Loading

Context

Change

myronmarston left a comment

Choose a reason for hiding this comment

myronmarston Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

acerbusace Nov 6, 2024

Choose a reason for hiding this comment

myronmarston Nov 7, 2024

Choose a reason for hiding this comment

acerbusace Nov 8, 2024

Choose a reason for hiding this comment

acerbusace commented Nov 1, 2024

acerbusace commented Nov 6, 2024

myronmarston left a comment

Choose a reason for hiding this comment

myronmarston Nov 7, 2024

Choose a reason for hiding this comment

myronmarston left a comment

Choose a reason for hiding this comment

myronmarston left a comment

Choose a reason for hiding this comment

myronmarston Nov 11, 2024

Choose a reason for hiding this comment

acerbusace Nov 11, 2024

Choose a reason for hiding this comment

acerbusace commented Nov 11, 2024

acerbusace commented Oct 29, 2024 •

edited

Loading

myronmarston Oct 29, 2024 •

edited

Loading