dssysolyatin commented on code in PR #4491:
URL: https://github.com/apache/calcite/pull/4491#discussion_r2255396686
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -2210,7 +2210,7 @@ private static String toSql(RelNode root, SqlDialect
dialect,
+ "INNER JOIN \"foodmart\".\"sales_fact_1997\" ON
\"product\".\"product_id\" = \"sales_fact_1997\".\"product_id\"\n"
+ "GROUP BY \"product\".\"product_id\"\n"
+ "HAVING COUNT(*) > 1) AS \"t2\"\n"
- + "WHERE \"t2\".\"product_id\" > 100";
+ + "WHERE \"product_id\" > 100";
Review Comment:
It’s a confusing, but I’ll try to explain it. It is kind of similar.
### Without fix
If you set a breakpoint inside the `visit(Filter filter)` method at this
line:
https://github.com/apache/calcite/blob/cfaadedd8fd34c480395f9e843a73bf4bb8fe459/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java#L486
...and then inspect `x.aliases` (where x is the SqlImplementor.Result for
the Project node), you'll see it contains both `product` and `sales_fact_1997`
(Join transfers aliases to SqlImplementor.Result for Project). This is
incorrect behavior because the node above the Project should not have access to
these aliases. (At least from my point of view)
Next, a new builder is created to transform the `Filter.condition`: (note:
At this point we already set mapping between correlation variable and context)
https://github.com/apache/calcite/blob/cfaadedd8fd34c480395f9e843a73bf4bb8fe459/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java#L487
When this new builder is created, it also creates a new context
`builder.context`. This context's `qualified` flag is set to `true` because the
previous `Result` `x` had multiple aliases (`aliases.length` > 1). Because it
`t2` is present
https://github.com/apache/calcite/blob/cfaadedd8fd34c480395f9e843a73bf4bb8fe459/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java#L1902-L1903
Interestingly, the new builder.context itself is valid and correctly
contains only the alias t2. This raises a good question: why can't this same
context be used for the correlation variable as well? Perhaps I'm
misunderstanding something.
### With fix
With the fix, `x.aliases` correctly contains only the `t2` alias. Because
there is now only one alias, the `qualified` flag for the new `builder.context`
is set to false. This is why the `t2` prefix is no used for product_id.
However, it's important to note that the new context itself is still correct
(`builder.context`). It contains also one alias = `t2`
--
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]
