calcite
calcite copied to clipboard
apache
CALCITE-4913 Correlated variables in a select list are not deduplicated
@korlov42 In order to facilitate the review, can you put in the description the SQL query along with the respective (problematic) plan? If it causes wrong results then please include expected and actual results. It would be good also to add a test in sub-query.iq or another similar place to ensure that the query actually runs and produces the expected results.
@korlov42 , What do you suggest for cases like https://github.com/apache/calcite/blob/f3c09365c2eecbb5198d9acbef638f76053a49a9/mongodb/src/main/java/org/apache/calcite/adapter/mongodb/MongoRules.java#L302-L318 ?
It looks like you did not refactor mongodb adapter, and I guess it would result in loss of semantics. It looks like variablesSet would be discarded.
The sad thing is Calcite users would get similar issues: they would get wrong results/wrong plans, and nobody would know the reason.
WDYT?
The sad thing is Calcite users would get similar issues: they would get wrong results/wrong plans, and nobody would know the reason.
Before this patch, they could get a correlated projection without being notified this particular project should set a correlation variable (project#getVariablesSet() returns an empty set). The only change is that now a project returns a valid set of variables it set.
@vlsi, I've added a @Deprecated on the Project's constructor. This highlighted a bunch of missed places (primarily, adapters). So all of them are fixed now, I suppose.
With this deprecation users should be aware of changes. But, IMO, despite now the project rel returns correlation variables, the semantic is still the same. Before this patch it was possible to set correlated scalar subquery as a projection expression. The only difference now is there is an easy way to extract all variables required by such a correlated scalar subqueries.
@korlov42 , I've lost interest in Calcite, and I have no time for reviewing the PR.
Hi @korlov42 , I am very busy in the next 10 days or so I don't think I will have time to review the latest changes immediately. After that however, if it is still un-merged please ping me again and I will try to get this in; it seems to be very close.
Hi, @zabetak! Seems like 10 days have passed. Now it's my turn to be unavailable for next 10 days, but I really hope you'll find time to review this.
Hi, @zabetak! Sorry for pushing, but is there any chance you'll find the time to help me get this PR done?
Thanks for the reminder @korlov42 , will try to get back to this in the following days. Apologies for the delay!