-
Notifications
You must be signed in to change notification settings - Fork 348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix unnecessary nesting of infix queries #1131
Conversation
|
||
import io.getquill.ast._ | ||
|
||
object ExpandMappedInfix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this will break user defined infix.map(a => f(a))
call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jilen Thanks for a review! Could you post an example? There's a test which handles that 6a94603#diff-e564af4387151d9877a17343b828b795R1050. ExpandMappedInfix
is applied before the latest Normalize
so user defined map will be normalized with expanded map in ExpandMappedInfix
. However if there's any known case please share and I will include them in test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mentegy For example, postgresql's Factorial
(or any other postfix operator)
infix("$i !").map(x => f(x))
Will this transformation break such operator ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jilen well that won't be possible since map
is not available for Quoted[T]
, e.g:
implicit class PostfixOps[T](v: T) {
def fact = quote(infix"$v!".as[T])
}
run {
q.map(e => e.id.fact) // no map available after fact
}
So no known for me places where it could be broken. But I've added additional restriction, see updated pattern matching below, hence transformation will be applied only if q
is a query in infix"$q ..."
These changes completely has broken spark module, @deusaquilus any ideas why? |
def apply(q: Ast): Ast = { | ||
Transform(q) { | ||
case Map(Infix("" :: parts, (q: Query) :: params), x, p) => | ||
Infix("" :: parts, Map(q, x, p) :: params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, transformation is applied only if q
is a query in infix"$q ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow seems like these changes fixed quill-spark errors, thank you @jilen !
The solution introduces special handling for when the infix starts with a query, which might be confusing to users and break existing queries. I think it's a reasonable tradeoff, though. Could you update the readme and the changelog accordingly? Thank you! |
* Fix unnecessary nesting of infix queries * Update readme and changelog
Fixes #1058
Problem
InfixContext
ofSqlQuery
always produces a nested query, sometimes this is unnecessary, see issue example.Solution
Unnest queries.
Checklist
README.md
if applicable[WIP]
to the pull request title if it's work in progresssbt scalariformFormat test:scalariformFormat
to make sure that the source files are formatted@getquill/maintainers