blackdrag commented on PR #3155:
URL: https://github.com/apache/groovy/pull/3155#issuecomment-6755933796
(1)
```Groovy
from e in 'employees'
join d in 'departments' on e.deptId == d.id
select e.name.reverse(), d.name.reverse() // double use of name
```
This should force mixed mode. The source records are reconstructed
independently, but their column labels originate from the SQL projection. If
the database returns duplicate column labels within a source projection, some
source information could be lost. The design assumes that JDBC metadata
provides a stable representation of the original source schema. That assumption
is not universally safe. JDBC drivers and database systems can differ in their
treatment of labels, duplicate names, case folding and quoting. naming the
columns could help, even though it requires remapping later. Also in that map
__ginq_mixed_key could collide with a database column of the same name.
There is also a lot about the mapping of the operations... I think a
documented list of intentional differences would help to understand not only
what is mapped, but also how semantics may differ (exact semantics depend on
the db of course if mapped to a db operation. Maybe something like:
```
Groovy operation: String.length()
SQL mapping: CHAR_LENGTH
Constraints:
- compatible character-counting semantics
- compatible treatment of null
- compatible type conversion
```
(2)
I think there are also a lot of issues around null-handling
```Groovy
def wanted = null
GQL(provider: 'native-sql', dataSource: db) {
from e in 'employees'
where e.deptId == wanted
select e.name
}
```
This will most likely result in `WHERE e.deptId = ?`, but it will not select
employees where deptId is null in the table. isNullLiteral fails to recognize
this, since this is not a null literal to begin with.
`where e.deptId !in 1..3` is translated to `NOT (e.deptId >= 1 AND e.deptId
<= 3)`. For a null department ID, the comparisons evaluate to UNKNOWN, and
negating UNKNOWN remains UNKNOWN. `e.deptId IS NULL OR (e.deptId < 1 OR
e.deptId > 3)` would represent Groovy semantics better.
I am not saying this must work according to Groovy semantics, but it needs
documentation when it does and when not.
(3)
```Groovy
def suffix = '!'
from e in 'employees'
select e.name + suffix
```
isStringy() does not recognize the captured string variable and silently
generates arithmetic addition. The implementation depends a lot on recognizes
cases like this, but cannot always do so. Maybe we should consider rejecting
some constructions. Having clear limits is better than unpredictable behavior
(4)
Currently it works like this: Groovy AST -> GinqToSqlTranslator -> SQL IR ->
Dialect-specific renderer -> SQL text
While reasonable, if a translation succeeds in producing a SqlFunction, a
particular dialect might still not support the corresponding function. FlOOR
and CEILING have been fixed, but I think the mixed-mode planner needs general
mechanism for considering dialect capabilities when determining which clauses
can be pushed down. Or there must be a way for the dialect renderer to fall
back to non-SQL implementation.
--
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]
