Skip to content

Phase 4: Use grouping_missing_value_placeholder in aggregation logic - #895

Merged
mmarston merged 1 commit into
mainfrom
mattmarston/phase-4-grouping-logic
Nov 6, 2025
Merged

Phase 4: Use grouping_missing_value_placeholder in aggregation logic#895
mmarston merged 1 commit into
mainfrom
mattmarston/phase-4-grouping-logic

Conversation

@mmarston

@mmarston mmarston commented Oct 30, 2025

Copy link
Copy Markdown
Contributor

This is 4th in a series of new pull requests that are working toward a new approach to handling missing values in subaggregations as described in #882. For an outline of the steps see the comment from @myronmarston here.

This commit refactors the grouping logic to use the grouping_missing_value_placeholder from types, falling back to separate missing aggregations when no placeholder is defined.

Backward compatibility:

  • When no placeholder is defined (returns nil), falls back to the old approach of creating separate missing aggregations
  • This ensures existing behavior is preserved for types without placeholders

Other PRs

Note PRs after the first one are on my fork since I don't currently have permissions to push to the block/elasticgraph repo (so I couldn't create stacked PRs on the block repo that are based on the branch from the prior PR).

Phase 1: Add grouping_missing_value_placeholder runtime metadata
Phase 2: Infer grouping_missing_value_placeholder based on mapping_type
Phase 3: Wire up grouping_missing_value_placeholder in GraphQL layer
Phase 4: Use grouping_missing_value_placeholder in aggregation logic (this PR)

@mmarston

Copy link
Copy Markdown
Contributor Author

To test this out and provide examples I used the demo app and modified the factory to generate some null values for fields of albums:

diff --git a/lib/demo_app/factories.rb b/lib/demo_app/factories.rb
index 2b4ae97..18a0e9c 100644
--- a/lib/demo_app/factories.rb
+++ b/lib/demo_app/factories.rb
@@ -13,7 +13,7 @@ FactoryBot.define do
     name { Faker::Music.band }
 
     lifetimeSales do
-      albums.map { |a| a.fetch(:soldUnits) }.sum
+      albums.map { |a| a.fetch(:soldUnits) }.compact.sum
     end
 
     bio { build(:artist_bio, artistName: name) }
@@ -114,9 +114,9 @@ FactoryBot.define do
 
   factory :album, parent: :hash_base do
     __typename { "Album" }
-    name { Faker::Music.album }
-    releasedOn { Faker::Date.between(from: "1950-01-01", to: "2025-12-31").iso8601 }
-    soldUnits { Faker::Number.between(from: 10000, to: 100_000_000) }
+    name { rand < 0.1 ? nil : Faker::Music.album }
+    releasedOn { rand < 0.1 ? nil : Faker::Date.between(from: "1950-01-01", to: "2025-12-31").iso8601 }
+    soldUnits { rand < 0.1 ? nil : Faker::Number.between(from: 10000, to: 100_000_000) }
     tracks do
       Faker::Number.between(from: 1, to: 15).times.map do |index|
         build(:album_track, trackNumber: index + 1)

GraphQL Query with a subaggregation grouping by a single field

First is an example that groups by a single fie.d

query SubAggregationExample {
  artistAggregations {
    nodes {
      count
      subAggregations {
        albums(first: 2) {
          nodes {
            groupedBy {
              name
}}}}}}}

GraphQL query results

{
  "data": {
    "artistAggregations": {
      "nodes": [
        {
          "subAggregations": {
            "albums": {
              "nodes": [
                {
                  "countDetail": {
                    "approximateValue": 80
                  },
                  "groupedBy": {
                    "name": null
                  }
                },
                {
                  "countDetail": {
                    "approximateValue": 12
                  },
                  "groupedBy": {
                    "name": "Boston"
}}]}}}]}}}

ES/OS query with missing bucket aggregation (without this PR)

Note there are two aggregations, albums.name and albums.name:m, the latter a missing bucket aggregation.

{
  "aggs": {
    "artistAggregations:albums": {
      "nested": {
        "path": "albums"
      },
      "aggs": {
        "albums.name": {
          "terms": {
            "field": "albums.name",
            "collect_mode": "depth_first",
            "size": 6,
            "show_term_doc_count_error": false
          },
          "meta": {
            "key_path": [
              "key"
            ],
            "merge_into_bucket": {},
            "grouping_fields": [
              "albums.name"
            ]
          }
        },
        "albums.name:m": {
          "missing": {
            "field": "albums.name"
          }
        }
      },
      "meta": {
        "buckets_path": [
          "albums.name"
        ],
        "size": 5,
        "adapter": "non_comp"
      }
    }
  },
  "size": 0,
  "track_total_hits": true,
  "_source": false
}

ES/OS query using missing value (with this PR)

Note there is only a single aggregation, albums.name, with a missing parameter.
Also note the addition of missing_values in the meta object.

{
  "aggs": {
    "artistAggregations:albums": {
      "nested": {
        "path": "albums"
      },
      "aggs": {
        "albums.name": {
          "terms": {
            "field": "albums.name",
            "collect_mode": "depth_first",
            "missing": "F0EMq6lVSf_4QNXH-34Tumqf",
            "size": 6,
            "show_term_doc_count_error": false
          },
          "meta": {
            "key_path": [
              "key"
            ],
            "merge_into_bucket": {},
            "missing_values": [
              "F0EMq6lVSf_4QNXH-34Tumqf"
            ],
            "grouping_fields": [
              "albums.name"
            ]
          }
        }
      },
      "meta": {
        "buckets_path": [
          "albums.name"
        ],
        "size": 5,
        "adapter": "non_comp"
      }
    }
  },
  "size": 0,
  "track_total_hits": true,
  "_source": false
}

GraphQL query grouping by two fields

query SubAggregations {
  artistAggregations {
    nodes {
      count
      subAggregations {
        albums(first: 5) {
          nodes {
            groupedBy {
              name
              soldUnits
}}}}}}}

GraphQL query results

{
  "data": {
    "artistAggregations": {
      "nodes": [
        {
          "subAggregations": {
            "albums": {
              "nodes": [
                {
                  "countDetail": {
                    "approximateValue": 9
                  },
                  "groupedBy": {
                    "name": null,
                    "soldUnits": null
                  }
                },
                {
                  "countDetail": {
                    "approximateValue": 1
                  },
                  "groupedBy": {
                    "name": null,
                    "soldUnits": 1890792
}}]}}}]}}}

ES/OS query with missing bucket aggregations (without this PR).

Note the nested permutations.
(meta field not shown)

{
  "aggs": {
    "artistAggregations:albums": {
      "nested": {
        "path": "albums"
      },
      "aggs": {
        "albums.name": {
          "terms": {
            "field": "albums.name",
            "collect_mode": "depth_first",
            "size": 6,
            "show_term_doc_count_error": false
          },
          "aggs": {
            "albums.soldUnits": {
              "terms": {
                "field": "albums.soldUnits",
                "collect_mode": "depth_first",
                "size": 6,
                "show_term_doc_count_error": false
              },
              "meta": ...
            },
            "albums.soldUnits:m": {
              "missing": {
                "field": "albums.soldUnits"
              }
            }
          },
          "meta": ...
        },
        "albums.name:m": {
          "missing": {
            "field": "albums.name"
          },
          "aggs": {
            "albums.soldUnits": {
              "terms": {
                "field": "albums.soldUnits",
                "collect_mode": "depth_first",
                "size": 6,
                "show_term_doc_count_error": false
              },
              "meta": ...
            },
            "albums.soldUnits:m": {
              "missing": {
                "field": "albums.soldUnits"
              }
            }
          },
          "meta": ...
        }
      },
      "meta": ...
    }
  },
  "size": 0,
  "track_total_hits": true,
  "_source": false
}

ES/OS query using missing value (with this PR)

Note there are fewer aggregations.
(meta field is shown here for full context, but that since meta is not shown above, this doesn't visually illustrate how much simpler this query is than the one above)

{
  "aggs": {
    "artistAggregations:albums": {
      "nested": {
        "path": "albums"
      },
      "aggs": {
        "albums.name": {
          "terms": {
            "field": "albums.name",
            "collect_mode": "depth_first",
            "missing": "F0EMq6lVSf_4QNXH-34Tumqf",
            "size": 6,
            "show_term_doc_count_error": false
          },
          "aggs": {
            "albums.soldUnits": {
              "terms": {
                "field": "albums.soldUnits",
                "collect_mode": "depth_first",
                "missing": "NaN",
                "size": 6,
                "show_term_doc_count_error": false
              },
              "meta": {
                "key_path": [
                  "key"
                ],
                "merge_into_bucket": {},
                "missing_values": [
                  "NaN"
                ],
                "grouping_fields": [
                  "albums.soldUnits"
                ]
              }
            }
          },
          "meta": {
            "buckets_path": [
              "albums.soldUnits"
            ],
            "key_path": [
              "key"
            ],
            "merge_into_bucket": {},
            "missing_values": [
              "F0EMq6lVSf_4QNXH-34Tumqf"
            ],
            "grouping_fields": [
              "albums.name"
            ]
          }
        }
      },
      "meta": {
        "buckets_path": [
          "albums.name"
        ],
        "size": 5,
        "adapter": "non_comp"
      }
    }
  },
  "size": 0,
  "track_total_hits": true,
  "_source": false
}

@mmarston

Copy link
Copy Markdown
Contributor Author

This example shows that ElasticSearch/OpenSearch support NaN as a missing value for numeric fields in terms aggregations. In this case, the albums.soldUnits field has a mapping type of integer. This shows that NaN works with integers, not just floats.

I ran these queries on both ElasticSearch and OpenSearch.

{
  "size": 0,
  "aggs": {
    "artistAggregations:albums": {
      "nested": {
        "path": "albums"
      },
      "aggs": {
        "albums.name": {
          "terms": {
            "field": "albums.soldUnits",
            "missing": "NaN",
            "size": 3
} } } } } }

Here is the query results

  "aggregations": {
    "artistAggregations:albums": {
      "doc_count": 931,
      "albums.name": {
        "doc_count_error_upper_bound": 10,
        "sum_other_doc_count": 833,
        "buckets": [
          {
            "key": "NaN",
            "doc_count": 96
          },
          {
            "key": 114968,
            "doc_count": 1
          },
          {
            "key": 377163,
            "doc_count": 1
          }
        ]
      }
    }
  }

If I try to remove the quotes around NaN like this:

"missing": NaN

I get this error (unsurprisingly, since that isn't valid JSON):

{
  "error": {
    "root_cause": [
      {
        "type": "json_parse_exception",
        "reason": "Non-standard token 'NaN': enable `JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 12, column: 27]"
      }
    ],
    "type": "json_parse_exception",
    "reason": "Non-standard token 'NaN': enable `JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 12, column: 27]"
  },
  "status": 400
}

If I try to use "missing": "unknown" (a string that isn't NaN) I get back this error:

"type": "number_format_exception",
"reason": "For input string: \"unknown\""

And if I try to use "missing": null I get back this error:

    "reason": "[12:24] [terms] failed to parse field [missing]",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "[missing] must not be null: [albums.name]"
    }

And if I try "missing": "null" (the string "null") I get back a number_format_exception like I did with "unknown".

A number as a string works and results in a number (not as a string) in the response bucket. With "missing": "-1" (number as JSON string) I got back these buckets:

"buckets": [
    {
    "key": -1,
    "doc_count": 96
    },
    {
    "key": 114968,
    "doc_count": 1
    },
    {
    "key": 377163,
    "doc_count": 1
    }
]

I get the same results as above when using "missing": -1 (as a number, without quotes).

mmarston referenced this pull request Oct 31, 2025
This commit refactors the grouping logic to use the grouping_missing_value_placeholder
from types, falling back to separate missing aggregations when no placeholder is defined.

Backward compatibility:
- When no placeholder is defined (returns nil), falls back to the old approach of creating
  separate missing aggregations
- This ensures existing behavior is preserved for types without placeholders
@mmarston
mmarston force-pushed the mattmarston/phase-3-graphql-wiring branch 3 times, most recently from 61eeb79 to f7ece77 Compare October 31, 2025 18:44
Base automatically changed from mattmarston/phase-3-graphql-wiring to main October 31, 2025 20:04
@mmarston
mmarston force-pushed the mattmarston/phase-4-grouping-logic branch from 762343b to 3274a96 Compare October 31, 2025 21:47
Comment thread elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/field_term_grouping.rb Outdated
Comment thread elasticgraph-graphql/spec/support/sub_aggregation_support.rb Outdated
Comment thread elasticgraph-graphql/spec/support/sub_aggregation_support.rb Outdated
Comment thread elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/query_spec.rb Outdated
@mmarston
mmarston force-pushed the mattmarston/phase-4-grouping-logic branch from 3274a96 to 83c5eb7 Compare November 4, 2025 02:56
This commit refactors the grouping logic to use the grouping_missing_value_placeholder
from types, falling back to separate missing aggregations when no placeholder is defined.

Backward compatibility:
- When no placeholder is defined (returns nil), falls back to the old approach of creating
  separate missing aggregations
- This ensures existing behavior is preserved for types without placeholders
@mmarston
mmarston force-pushed the mattmarston/phase-4-grouping-logic branch from 83c5eb7 to 5781708 Compare November 4, 2025 22:54
@mmarston

mmarston commented Nov 4, 2025

Copy link
Copy Markdown
Contributor Author

I believe I've addressed all comments. Since this is the only PR in this series that actually changes query behavior, I'm OK if we hold off on merging it until we've been able to deploy in a staging environment to verify it improves performance without introducing issues.

@myronmarston myronmarston left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment thread elasticgraph-graphql/spec/support/sub_aggregation_support.rb Outdated
@mmarston

mmarston commented Nov 6, 2025

Copy link
Copy Markdown
Contributor Author

Here is picture showing the improvements as a result of this PR. This is p99.99 latency (plus p99.9, p99) and the deployment of this PR is where the graph smooths out.

image (2)

@mmarston
mmarston merged commit e0988be into main Nov 6, 2025
18 checks passed
@mmarston
mmarston deleted the mattmarston/phase-4-grouping-logic branch November 6, 2025 18:46
@myronmarston

Copy link
Copy Markdown
Collaborator

Here is picture showing the improvements as a result of this PR. This is p99.99 latency (plus p99.9, p99) and the deployment of this PR is where the graph smooths out.

image (2)

Woah, that's massive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants