Issue #2767 - EQL: portable aggregation over subqueries#2774
Open
homedirectory wants to merge 59 commits into
Open
Issue #2767 - EQL: portable aggregation over subqueries#2774homedirectory wants to merge 59 commits into
homedirectory wants to merge 59 commits into
Conversation
…rbitrary expressions This commit also contains a lot of related changes to EQL AST nodes, primarily oriented at update/replace operations.
Deterministic order is desired for EQL tests that assert shapes of queries. Several tests were failing, including ImplicitlyCalculatedPropertiesTest.expression_for_one_2_one_property_of_persistent_entity_type_declared_and_available_at_synthetic_entity_with_id_is_correct, because the "expected" and "actual" queries had yields ordered differently while being equal otherwise. The constructor of Yields2 stores yields in a sorted map, ordered by alias. The previous approach would generate random aliases, resulting in non-deterministic order.
Let's avoid all that complexity associated with using collectProps.
Previously, if a query contained two aggregations -- one over a
persistent property and another over a more complex operand -- the
former would be skipped, as it is a simple aggregation not requiring a transformation,
but the latter would trigger the transformation.
Because the former would be skipped, and extractProperties would not pick
it up (it does not descend into aggregations), the transformation would
not replace it, resulting in an invalid property reference.
For example:
select(TgFuelUsage.class)
.yield().sumOf().prop("qty").as("totalQty")
.yield().sumOf().beginExpr().prop("qty").mult().prop("price").endExpr().as("cost")
.modelAsAggregate()
==>
select(select(TgFuelUsage.class)
.yield().beginExpr().prop("qty").mult().prop("price").endExpr().as("c1")
.modelAsAggregate())
.yield().sumOf().prop("qty").as("totalQty") // INVALID: No such property "qty"
.yield().sumOf().prop("c1").as("cost")
.modelAsAggregate()
…ail with SQL Server
Having the transformation at stage 2 brings a limitation: calculated properties cannot be transformed. Transforming a calculated property requires replacing it with a transformed expression. E.g., if the calculated expression is `sum(x)`, expressed as `prop(total)`, the resulting AST will have to contain `sum(c1)`, where `c1` denotes the original `x` materialised in a source query. This means that calculated properties have to be expanded. At the same time, the canonical expansion already occurs during Stage 2 -> 3 transformation (Prop2.transform). Stage 3 is a better choice because all calculated properties have already been expanded.
Using null instead of empty() is more convenient for testing. The plan is to replace all nulls with empty nodes or the Optional type.
All tests had to be refactored to craft the expected query's AST by hand, because matching generated SQL aliases is tricky.
…ages IDEA detects ComparisonFailure and presents a diff window "Expected vs Actual".
All record components should be taken into account. This is consistent with other nodes that contain generated IDs, such as AbstractSource3.
Unlikely in practice, but consistent with the current model. Usage analysis shows that yields may be null only in tests. The present model will likely be simplified in the future to unify "can be null" and "can be empty" for each query component.
…stead The new implementation of Prop3.hashCode and equals is consistent with Prop2. It also makes sense: two properties are equal only if they come from the same source, which are uniquely identified by their IDs.
Arguments of aggregate functions occurring only in an order-by were not materialised, leaving the outer order-by referencing the original source, which the outer query no longer accesses.
…ies. The replacement operation does not descend into subqueries. A subquery in a yield or an order-by outside of an aggregate argument would keep referencing the original source, which the outer query no longer accesses, resulting in an invalid query. The transformation is now skipped for such queries. SQL Server evaluates them natively unless they genuinely require materialisation, in which case it rejects them natively -- the same outcome as in the absence of the transformation. The limitation of a subquery that is both grouped by and yielded becomes an instance of this rule.
A concatOf order item that references a yield has neither an operand nor a yield at stage 3, as ConcatOf2 resolves yield references against empty yields. Streaming the children of a concatOf NPEd on the null operand of such an item. Also, once any order item matched the replacement map, every non-matching item had its operand replaced with null; non-matching items are now left untouched.
… an aggregation. The applicability check did not verify the presence of an aggregation, contradicting the documented contract: any group-by with a non-trivial operand triggered the rewrite, adding needless query nesting. Group-by operands alone no longer trigger the transformation. Aggregations are detected as aggregate functions rather than through their extracted arguments, so that count(*), which has no argument, still makes a query eligible. This preserves the materialisation of group-by subqueries in queries that yield only count(*).
Prop3.hashCode guarded against a null source while equals dereferenced it unconditionally. A null source has never been legitimate: all construction sites resolve an existing source, and Prop3.sql would fail on a null source regardless. The invariant is now enforced at construction, and the vestigial null guard is removed.
…run. For a test method inherited from a base class, the annotation lookup used the method's declaring class, missing a class-level WithDbVersion on the concrete test class and running DB-specific tests in other environments. The lookup now uses the test class being run. WithDbVersion is also made @inherited, so a class-level annotation on a base test class applies to its subclasses.
01es
approved these changes
Jul 9, 2026
Had to adjust yield order for a test.
The inherited AbstractSingleOperand3.type can be different for two otherwise equal Prop3 instances.
For example, this query was encountered:
groupBy().prop("inventory")
yield().prop("inventory").as("inventory") -- PropType(Java=Inventory, Hibernate=Long)
yield().prop("inventory.id").as("id") -- PropType(Java=Long, Hibernate=Long)
If we consider these properties not equal, the query coming out of AggregateOperandMaterialiser will be invalid.
prop("inventory") in groupBy and yield()...as("inventory") will be materialised under one column as expected,
but not yield()...as("id"), which will remain in the outer query, referencing the source that it can no longer access.
This reverts commit 5e9e89c. This change is fragile: it may affect the process of building EntityTreeResult, resulting in an entity-typed yield treated as primitive-typed. Further analysis is required to understand how to best address the core issue.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolve #2767.
To be completed by the pull request creator
This section should be completed with reference to section Preparing PR of the Code and PR reviews wiki page.
Create the pull request as a draft by tapping the dropdown arrow on the 'Create pull request' button under the pull request description (below the text box where this description is being edited) and changing the default
Create pull requesttoDraft pull request.Or, if the pull request has already been created, convert it to draft by tapping the "Convert to draft" link beneath the "Reviewers" section.
A self-review of all changes has been completed, and the changes are in sync with the issue requirements.
Changes to the requirements have been reflected in the issue description.
Any "leftovers" such as sysouts, printing of stack traces, and any other "temporary" code, have been removed.
Minor refactorings, such as renamings, extraction of constants, etc., have been addressed.
Developer documentation (e.g., comments, Javadoc), have been provided where required.
New Java tests have been written or existing tests adjusted, if required, to cover the new functionality.
All existing and new Java tests pass successfully by running them with Maven.
All existing and new Web tests pass successfully.
Established security practices have been followed, including the existence and attribution of security tokens.
Changes have been inspected for possible NPE situations, and the changes are sufficiently defensive.
The correct base branch has been selected for these changes to be merged into.
The latest changes from the base branch have already been merged into this feature branch (and tested).
Added a change overview to the issue description or as a wiki page, referenced in the issue description.
Some issues might be very descriptive and serve in place of a wiki page.
In such cases consider adding label
Wiki liketo the issue.Changes subject to performance considerations have been evaluated, and tested against production-size data if applicable.
This pull request does not contain significant changes, and at least one appropriate reviewer has been selected.
The
In progresslabel has been removed from the issue.The
Pull requestlabel has been added to the issue.The pull request has been made ready for review by tapping the "Ready for review" button below the list of commits on the pull request page.
To be completed by the pull request reviewer
This section should be completed with reference to section Performing PR review of the Code and PR reviews wiki page.
The
In progresslabel has been added to the pull request in GitHub.The issue requirements have been read and understood (along with any relevant emails and/or Slack messages).
The correct base branch is specified, and that base branch is up-to-date in the local source.
The issue branch has been checked out locally, and had the base branch merged into it.
All automated tests pass successfully.
Ensure the implementation satisfies the functional requirements.
Ensure that code changes are secure and align with the established coding practices, including code formatting and naming conventions.
Ensure that code changes are documented and covered with automated tests as applicable.
Ensure that code changes are well-suited for informal reasoning.
Ensure that changes are documented for the end-user (a software engineer in the case of TG, or an application user in the case of TG-based applications).
If there are significant changes (described above), special attention has been paid to them.
Marked the task items in section "Significant changes" as completed to indicate that corresponding changes have been reviewed, improved if necessary, and approved.
The issue or issues addressed by the pull request are associated with the relevant release milestone.
To be completed by the pull request reviewer once the changes have been reviewed and accepted
The changes have been merged into the base branch (unless there is a specific request not to do so, e.g., they are to be released to SIT).
The issue branch has been deleted (unless the changes have not been merged - see above, or there is a specific request not to do so).
The
In progresslabel has been removed from the pull request.The
Pull requestlabel has been removed from the issue.