The PR needs to fix a bug or implement a feature. So, first you should log a JIRA case describing what doesn’t work. Write tests for what doesn’t work that you want to make work. (Or maybe you can refactor/generalize existing tests.) Then submit a PR, and we will review that PR on the basis of whether it fixes the bug.
That may sound like a lot of process. But the alternative is people just randomly changing code. > On Jun 3, 2021, at 7:37 AM, stanilovsky evgeny <estanilovs...@gridgain.com> > wrote: > > Julian, thanks for reply and comments. > Can you explain, is it would possible to commit PR containing deduplication > code moved upper this flag ? > If so - i will create PR and rerun all existing tests, of course. > Thanks ! > >> Master is a moving target. Apparently you are using a version of the >> code from sometime in March. These links work better: >> >> [1] >> https://github.com/apache/calcite/blob/796675c9b33e0461bc45a72780162d474a4b098b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L2881 >> >> [2] >> https://github.com/apache/calcite/blob/796675c9b33e0461bc45a72780162d474a4b098b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L6209 >> >> I don't know the exact cause of what you are seeing, but when you set >> "expand=false" you are choosing a newer (and better) code path. We do >> not have feature parity between the code paths. Nor do we write tests >> for both code paths. >> >> Some day I'd like to officially deprecate, then obsolete, >> "expand=true". It is de facto deprecated because we put more effort >> into the "expand=false" path. >> >> Julian >> >> >> On Fri, May 28, 2021 at 1:19 AM stanilovsky evgeny >> <estanilovs...@gridgain.com> wrote: >>> >>> Hi, calciters ) >>> >>> I am trying to figure out why DeduplicateCorrelateVariables [1] is called >>> only if withExpand [2] flag is true ? Why we don`t need to deduplicate in >>> appropriate case ? >>> >>> [1] >>> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L2881 >>> >>> [2] >>> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L6209 >>> >>> Thanks !
