Fix Infix causing ignoring renamings

[letalvoj]

Fixes #1138

Problem

Pattern matching in RenameProperties expected Query even in cases where an Ast could slip trough.

Solution

Use Ast instead of Query in partial functions in RenameProperties.applySchema and add the Infix case there.

Notes

The tests locally failed with an error regarding some DB connection. Probing?
I hope that PR will trigger CI. I'll check the checklist as soon as I know the tests pass.

Checklist

• Unit test all changes
• Update README.md if applicable
• Add [WIP] to the pull request title if it's work in progress
• Squash commits that aren't meaningful changes
• Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

[letalvoj]

@getquill/maintainers please see the schema aggregation. I am not sure about that. The following support only only one param in infix, which is not enough

      case Infix(parts, List(param)) =>
        val (p, schema) = applySchema(param)
        (Infix(parts, List(p)), schema)

yet the following does not work

      case Infix(parts, params) =>
        val (ps, schemas) = params.map(applySchema).unzip
        (Infix(parts, ps), Tuple(schemas))

and fails RenamedAllowFilteringSpec with

Expected :"SELECT time, [srcFreq, dstF]req FROM Table ALLOW..."
Actual   :"SELECT time, [freq, f]req FROM Table ALLOW..."

Since Tuple(schemas) does not seem to be enough, how to aggregate the schemas then?

[letalvoj]

Well, it seems that unconstrained infixing is too general to solve anyway ... For example, it seems that

infix"${query1} WHERE column IN ${query2}"

with column renamings is not Dealiased properly (the Ident is taken from query2). Should I be concerned with it or is fixing the simple case with a single query enough?

[mosyp]

Amount all of the changes only these 3 lines are actual fix, is it correct?
If this is true, you don't need to change the definition of applySchema from Query to Ast, since this breaks the original idea afaik (plus this introduce the user unfriendly exception).
As mentioned before, you can use approach as in https://github.com/getquill/quill/pull/1134/files#diff-1fd78b0579cdd5318ca84f4c836dd991R15. But since there's no apply(Infix):Infix in a StatelessTransformer, you can override the apply(Ast):Ast and then body would look like:

case Infix(..) => infix fix logic
case other => super.apply(other)

Such pattern is commonly used among the implementations of StatelessTransformer

[mosyp]

Oh sorry, now I see why this is needed. Also matching only single ast in tuple could be acceptable if we need a fix sine don't have a workaround. But first, I'll check it myself

[letalvoj]

👍 thanks a lot ;-)

[mosyp]

@letalvoj I've made it working with (placed right after case Map(q: Operation...):

case Map(Infix(parts, params), x, p) =>
        val schema = params.collect {
          case q: Query => applySchema(q)._2
        } match {
          case List(e) => e
          case ls      => Tuple(ls)
        }
        val replace = replacements(x, schema)
        val pr = BetaReduction(p, replace: _*)
        val prr = apply(pr)

        (Map(Infix(parts, params), x, prr), schema)

The idea is just like yours but this one addresses your concerns about multiple schemes and prevents changing the definition of applySchema. Feel free to use it in your PR.

Also I have a note about unit test you've added, please remove those 2 files you've created and place the tests under io.getquill.context.sql.norm.RenamePropertiesSpec to follow conventions

[letalvoj]

Cool, thanks. 👍 There is an issue with the method being longer than 50 lines. I do not actually like the idea of splitting it, it makes sense like it is. Is there a way to tell the code quality tool to ignore it?

I'll move the tests and try to write more tests to understand which cases it actually solves.

[mosyp]

@letalvoj Did you mean this one https://app.codacy.com/app/fwbrasil/quill/pullRequest?prid=2235554#new-file-issues-22885005006 ? Just ignore it (not a tool but method length)

[letalvoj]

Still a bit ambigous. If I am supposed to force codacy to ignore the particular issue, then I do not know how. The only way I found was to ignore the whole file 🤦‍♂️

https://support.codacy.com/hc/en-us/articles/360005097654-Ignore-files-from-Codacy-analysis

[mosyp]

@letalvoj I meant to ignore it by yourself, we'll be able to merge PR if CI and coverage passes.

[letalvoj]

@mentegy hey, sorry for the delay ;-) I've pushed the changes you proposed and moved the test. Thanks again.

[mosyp]

Great! Thanks =)