-
Notifications
You must be signed in to change notification settings - Fork 33
Alternative missing value approach to subaggregations #882
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,12 +11,29 @@ | |
| module ElasticGraph | ||
| class GraphQL | ||
| module Aggregation | ||
| class FieldTermGrouping < Support::MemoizableData.define(:field_path) | ||
| # @dynamic field_path | ||
| class FieldTermGrouping < Support::MemoizableData.define(:field_path, :field) | ||
| # @dynamic field_path, field | ||
| include TermGrouping | ||
|
|
||
| private | ||
| # Random 18 bytes converted to base64. Used 18 bytes instead of 16 to avoid base64 padding. | ||
| # Since it uses more bytes, this has a lower probability of collission than a 16 byte random UUID. | ||
| MISSING_STRING_PLACEHOLDER = "f1TXKoApWwIG3U8ks9vVduvU" | ||
| MISSING_NUMERIC_PLACEHOLDER = "NaN" | ||
|
|
||
| def missing_value_placeholder | ||
| unwrapped_type = field.type.unwrap_fully | ||
| case unwrapped_type.name | ||
| when "String", "ID" | ||
| MISSING_STRING_PLACEHOLDER | ||
| when "Int", "JsonSafeLong", "Float" | ||
| MISSING_NUMERIC_PLACEHOLDER | ||
| else | ||
| unwrapped_type.enum? ? MISSING_STRING_PLACEHOLDER : nil | ||
| end | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned about the approach here: this isn't a complete list of scalar types (for example, I'd prefer that we "invert" this, where scalar types themselves would define what their missing value placeholder is, and this would then just use the type's missing value, if there is one. Here's a sketch of how that could work: In schema.scalar_type "JsonSafeLong" do |t|
# ...
t.grouping_missing_value_placeholder "NaN"
endIn In Finally, you could read the Note: if we go this route, I'd ask that you break it up into multiple PRs as that's going to be way too big to easily review. As a suggested breakdown:
(If some of these PRs seem really small, feel free to combine them.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I intend to.
Yes, I see that as a better approach. Regardless of how/where we define the missing value placeholder, we'll continue to support data types for which we haven't defined a missing value placeholder. Before doing all the work to create 4 PRs as you've outlined it, I want to have confidence that making these changes will alleviate the performance/availability issues we're experiencing due to the complex generated queries. |
||
| end | ||
|
|
||
| private | ||
|
|
||
| def terms_subclause | ||
| {"field" => encoded_index_field_path} | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -31,6 +31,7 @@ def composite_clause(grouping_options: {}) | |||||
|
|
||||||
| def non_composite_clause_for(query) | ||||||
| clause_value = work_around_elasticsearch_bug(terms_subclause) | ||||||
| clause_value["missing"] = missing_value_placeholder if handles_missing_values? | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
As a general rule, we favor a coding style that avoids mutating objects unless strictly necessary. Re-assigning a local variable to a different object is fine though--local variables, by their nature, are localized.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll watch out for that. There are probably a couple places in this PR that mutate instead of re-assign. |
||||||
| { | ||||||
| "terms" => clause_value.merge({ | ||||||
| "size" => query.paginator.requested_page_size, | ||||||
|
|
@@ -39,6 +40,14 @@ def non_composite_clause_for(query) | |||||
| } | ||||||
| end | ||||||
|
|
||||||
| def missing_value_placeholder | ||||||
| nil | ||||||
| end | ||||||
|
|
||||||
| def handles_missing_values? | ||||||
| missing_value_placeholder != nil | ||||||
| end | ||||||
|
|
||||||
| INNER_META = {"key_path" => ["key"], "merge_into_bucket" => {}} | ||||||
|
|
||||||
| def inner_meta | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd sooner maybe expect a collision with NaN than maybe some random 64-bit number high up... but ES does support shorts/bytes/half_floats etc. Potentially we could make this PR string-only and probably still get 80% of the value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the general approach of only taking this approach for data types that have high confidence of avoiding a chance of collision.
For numeric types, the more I look into it, the more confident I am that we can use NaN without collision. JSON doesn't have a way to serialized NaN. And the top Google results say that NaN can't be stored in ElasticSearch. I tested trying to store NaN in a
doublefield in OpenSearch and when I tried"quantityFloat": NaNthen OpenSearch returns afailed to parseerror with reasonNon-standard token 'NaN'. And when I tried"quantityFloat": "NaN"it failed with reasonDouble value passed as String.Even if someone solved how to send NaN in JSON and how to store NaN in ElasticSearch or OpenSearch, we could confidently use NaN for ElasticGraph data types Int, JsonSafeLong, and LongString because ElasticGraph would reject NaN when validating those data types.
One risk with NaN is that ElasticSearch or OpenSearch could change to no longer support
"NaN"formissingvalue aggregation. But that would be a breaking change, so seems unlikely that they would do so.Another risk is that ES/OS could eventually support storing NaN, which would then open up the possibility of collision.