jdaugherty commented on code in PR #16733:
URL: https://github.com/apache/grails-core/pull/16733#discussion_r4360834011
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/JpaQueryContext.java:
##########
@@ -173,6 +175,7 @@ public Expression<?> getAliasedExpression(String alias) {
return expr;
}
+ @Override
public Expression<?> getFullyQualifiedExpression(String path) {
Review Comment:
I probed this at 1e47f7ed5d to see what each lookup returns for a
`sqlProjection` alias, with `width * 10 as scaled` where no `scaled` property
exists and `width * 10 as height` where `height` is a property:
| alias | `getFullyQualifiedExpression` | `getSelectionAlias` | built-in
`order(alias)` | built-in `eq(alias, …)` |
|---|---|---|---|---|
| `scaled` | `null`; `cb.desc(null)` then fails with "Ambiguous method
overloading for SqmCriteriaNodeBuilder#desc" | the projection | `order by 1
desc` | "Cannot use comparison criteria on non-existent property [scaled]" |
| `height` | the property, `order by spb1_0.height desc` | the projection |
`order by 1 desc` | `where spb1_0.height=?` |
For `sqlGroupProjection 'width, sum(height) as total'`, a handler using
`getSelectionAlias('total')` gives `order by 2 desc`, the same SQL as
`order('total', 'desc')`.
The two built-in columns of that table are not an accident of the
translator. They are the rule SQL, Hibernate 5 and this module already follow:
- SQL scopes a select alias to ORDER BY, where it wins over a column of the
same name, and never to WHERE. On H2 2.5.250, the test database: `select width
* 10 as height from t order by height desc` returns 40, 20; `... where height =
9` restricts the column; `... where scaled = 20` fails with `Column "SCALED"
not found`.
- Hibernate 5's Criteria API, which the plugin in #16563 is porting from,
does the same. `Order.toSqlString` resolves through
`CriteriaQueryTranslator.getColumnsUsingProjection`, which first looks for a
projection alias and then for a property; `SimpleExpression.toSqlString`,
behind `eq` and the other restrictions, resolves through `findColumns`, which
looks for a property only (hibernate-core 5.6.11).
- This module encoded it when `sqlProjection` landed: `assignOrderBy` checks
the `sqlProjection` alias before the property, and `registerSelectionAlias`
keeps the alias out of property resolution, with "Column aliases never shadow
properties in restrictions" in cb1f659bd0 (#16548).
Folding the selection into `getFullyQualifiedExpression` changes that
behavior, because the built-in predicates resolve through the same method and
`assignCriteria` runs after the projections are registered. Selection first
makes `eq('height', 9)` restrict the projection instead of the column on every
query whose projection alias matches a property, which is the shadowing the
commit above rules out and SQL does not do. Selection as a fallback keeps `eq`
as it is, but then a custom order resolves `height` to the property where
`order('height')` resolves it to the projection, so the built-in order and the
handler order the same alias differently.
Two lookups avoid both. An order handler that checks `getSelectionAlias`
first and `getFullyQualifiedExpression` second orders exactly as `order(alias)`
and as Hibernate 5's `Order` do, and a criterion handler that uses
`getFullyQualifiedExpression` alone restricts exactly as `eq` does. So I would
keep the accessor this commit adds rather than change the resolver.
What is wrong at this commit is the contract. The
`QueryPathContext.getFullyQualifiedExpression` javadoc says it takes "the alias
of a projection" and returns "the expression an alias or projection stands
for". That holds for an alias the translator registers with `registerAlias` and
not for the column alias of a `sqlProjection`, which is where this thread
started. Could the javadoc say which aliases it resolves and point to
`getSelectionAlias` for the column alias of a `sqlProjection` or
`sqlGroupProjection`, and could the guide's order-handler paragraph say to
check `getSelectionAlias` first, the way `order(alias)` does?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
