vlsi commented on PR #5291: URL: https://github.com/apache/calcite/pull/5291#issuecomment-5827869354
Thanks for the HAVING fix and for taking the comment suggestions. 8f2ab7d adds the `mapRootRelToFieldProjection` entry and `testNotInCorrelatedNullableHaving`, which covers that case. You wrote: "Some of these extra tests would be nice, but they are not really in scope of the issue being solved." Which of the four do you consider out of scope, and why? I agree for some of them and not for others, so I'd like to understand where you draw the line. Here is my view of each: 1. **Composite key.** I think this one is in scope. The PR replaces `joinInfo.leftKeys` with the first `keyCount` fields of `rex`, and with a single key the test cannot tell whether that count or the order of the keys is right. One query with `(t.mgr, t.deptno) not in (...)` would cover it. 2. **Plan before decorrelation.** I think this one is in scope too. The new code in `translateIn` walks two nested `LogicalCorrelate` nodes, but the only snapshot is the decorrelated plan, which has no `LogicalCorrelate` at all. A second test method with the same query and `.withDecorrelate(false)` would record the shape the fix handles. 3. **HAVING.** Done, thanks. 4. **Two correlated `NOT IN` in one query.** I agree this one can go to a later change. `translateIn` depended on the shape of the tree before this PR as well, so the test belongs with a change that touches that dependency. On the ON clause: I agree it is a separate issue. Is it the follow-up you plan to file? If not, I can file it. The PR description still says that `translateIn` "was assuming that the Join nodes are never Correlates" and that the change handles "both Join and Correlate nodes uniformly". On `main` the crash happens earlier, in `Blackboard.register` → `Correlate.isValid`, and the ON clause still fails, so neither sentence matches the change. My review has a suggested wording, if it helps. -- 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]
