HanumathRao commented on PR #3640: URL: https://github.com/apache/calcite/pull/3640#issuecomment-1937901907
> Thanks for your patience and the thorough explanation @HanumathRao , I really appreciate it. > > I understand a bit more the situation now... and I beg to differ. IMHO fix 2 is just "sweeping the problem under the rug", we could avoid the problem in `Prepare.java`, but the underlying issue still remains. Plus, before removing this [Chesterton's fence](https://en.wiktionary.org/wiki/Chesterton%27s_fence), we would need to understand the reasons why field trimming was included in there in the first place. Also, removing the trimming can have undesired consequences (e.g. some projects might have fields which are expensive to retrieve/compute, so they should only be part of the final plan if they are really used, otherwise they should be trimmed; so this change would lead to a performance regression). > > I think fix 1 is the right way to go (although I understand it will be harder). Field trimming is quite a powerful tool (not without some bugs), which can be used by downstream projects (either via SqlToRelConverter#trimUnusedFields or directly via RelFieldTrimmer, since both are public); so if we find a bug on it, I think it would be better to try to fix it. > Thanks for your patience and the thorough explanation @HanumathRao , I really appreciate it. > > I understand a bit more the situation now... and I beg to differ. IMHO fix 2 is just "sweeping the problem under the rug", we could avoid the problem in `Prepare.java`, but the underlying issue still remains. Plus, before removing this [Chesterton's fence](https://en.wiktionary.org/wiki/Chesterton%27s_fence), we would need to understand the reasons why field trimming was included in there in the first place. Also, removing the trimming can have undesired consequences (e.g. some projects might have fields which are expensive to retrieve/compute, so they should only be part of the final plan if they are really used, otherwise they should be trimmed; so this change would lead to a performance regression). > > I think fix 1 is the right way to go (although I understand it will be harder). Field trimming is quite a powerful tool (not without some bugs), which can be used by downstream projects (either via SqlToRelConverter#trimUnusedFields or directly via RelFieldTrimmer, since both are public); so if we find a bug on it, I think it would be better to try to fix it. Thank you, @rubenada, for your comments. I have addressed the issue in RelFieldTrimmer to ensure that correlated variables in the subqueries are not trimmed. Nevertheless, I maintain my stance that executing RelFieldTrimmer in this context might be unnecessary. Since RelFieldTrimmer is run as a separate program after the SubQueryRemoval Rule ([link](https://github.com/apache/calcite/blob/8fd9518887302af43e7e74cdb155474dcebfc9f5/core/src/main/java/org/apache/calcite/tools/Programs.java#L290)), calling it in Prepare.java may not yield any benefits. Kindly review the changes at your earliest convenience. -- 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]
