Adds nested query and aggregation support #143
Draft
+619
−156
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds support for nested queries and aggregations. This change allows the parser to automatically wrap nested fields in nested queries and aggregations, simplifying the query and aggregation building process. Adds multiple tests to ensure that the nested queries and aggregations are built correctly.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces nested query and aggregation support to the Foundatio.Parsers.ElasticQueries library, allowing automatic wrapping of nested fields in appropriate Elasticsearch nested queries and aggregations to simplify the query building process.
- Adds comprehensive test coverage for nested field handling in queries, aggregations, and mixed operations
- Includes tests for various scenarios including single fields, multiple fields, range queries, and filtering operations
- Validates that the parser correctly wraps nested field operations in proper Elasticsearch nested contexts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Foundatio.Parsers.ElasticQueries.Tests/ElasticQueryParserTests.cs | Adds 9 comprehensive test methods covering nested query and aggregation functionality |
| src/Foundatio.Parsers.ElasticQueries/Visitors/CombineAggregationsVisitor.cs | Adds a TODO comment for future investigation |
| var termNode = child as TermNode; | ||
| if (termNode != null && termsAggregation != null) | ||
| { | ||
| // Look into this... |
There was a problem hiding this comment.
The comment 'Look into this...' is vague and doesn't provide clear guidance about what needs investigation. Consider replacing it with a more specific description of what needs to be looked into or remove it if it's not necessary.
Suggested change
| // Look into this... |
Copilot uses AI. Check for mistakes.
niemyjski
commented
Jul 21, 2025
tests/Foundatio.Parsers.ElasticQueries.Tests/ElasticQueryParserTests.cs
Outdated
Show resolved
Hide resolved
niemyjski
commented
Jul 21, 2025
| var processor = new ElasticQueryParser(c => c.SetLoggerFactory(Log).UseMappings<MyNestedType>(Client).UseNested()); | ||
|
|
||
| // Act | ||
| var result = await processor.BuildAggregationsAsync("terms:(nested.field1~@include:apple,banana,cherry nested.field4~@include:1,2,3)"); |
There was a problem hiding this comment.
I think this syntax is wrong: "terms:(field1 @include:myinclude )
niemyjski
commented
Jul 21, 2025
| var processor = new ElasticQueryParser(c => c.SetLoggerFactory(Log).UseMappings<MyNestedType>(Client).UseNested()); | ||
|
|
||
| // Act | ||
| var result = await processor.BuildAggregationsAsync("terms:(nested.field1 @exclude:myexclude @include:myinclude @include:otherinclude @missing:mymissing @exclude:otherexclude @min:1)"); |
There was a problem hiding this comment.
I think this needs to be simplified
Updates query parser to correctly handle include/exclude lists for nested fields. - Corrects an issue where nested field includes were not properly parsed. - Modifies the expected type for include/exclude values to string for consistency. - Converts relevant tests to async and updates assertion logic to match expected behavior.
Moves the MyNestedType class definition from ElasticQueryParserTests to ElasticNestedQueryParserTests. This change centralizes the type definition where it is primarily used and avoids code duplication, improving code organization and maintainability.
Updates the tests and nested query parsing logic to correctly reference the `MyNestedType` class, resolving type resolution issues. Additionally, renames one of the nested query parser tests for clarity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related FoundatioFx/Foundatio.Repositories#150
Adds support for nested queries and aggregations. This change allows the parser to automatically wrap nested fields in nested queries and aggregations, simplifying the query and aggregation building process.
Adds multiple tests to ensure that the nested queries and aggregations are built correctly.