[CALCITE-6778] SOME rewrite for correlated queries does not handle null values correctly #4144
Conversation
| @@ -405,8 +405,9 @@ private static RexNode rewriteSome(RexSubQuery e, Set<CorrelationId> variablesSe | |||
| // from emp group by name) as q on e.name = q.name | |||
| builder.push(e.rel) | |||
| .aggregate(builder.groupKey(), | |||
| builder.count(true, "c"), | |||
| builder.count(true, "d", builder.field(0)), | |||
| builder.count(false, "c"), | |||
There was a problem hiding this comment.
is the comment above describing the rewrite accurate?
There was a problem hiding this comment.
oops, forgot to update it. nice catch
There was a problem hiding this comment.
is this enough? where is dd used?
There was a problem hiding this comment.
@NobiGo did you happen to author this code?
Then perhaps you are the most suitable person to approve.
There was a problem hiding this comment.
these would be very nice comments to add in the code.
racevedoo
force-pushed
the
fix-calcite-6778
branch
2 times, most recently
from
641299e to
398e761
Compare
| @@ -392,21 +392,22 @@ private static RexNode rewriteSome(RexSubQuery e, Set<CorrelationId> variablesSe | |||
| // then false // sub-query is empty for corresponding corr value | |||
| // when q.c = 0 then false // sub-query is empty | |||
| // when e.deptno is null then unknown | |||
| // when q.c <> q.d && q.d <= 1 | |||
| // when q.c <> q.d && q.dd <= 1 | |||
There was a problem hiding this comment.
I haven't tested it, but should it bewhen q.d <> q.dd && q.d <= 1 here?
I guess what is trying to be expressed here is the scenario when deptno is null
There was a problem hiding this comment.
the q.c <> q.d comparison identifies if there are any null values, since count(*) counts null values and count(deptno) does not.
the q.dd <= 1 part (previously q.d <= 1) remains the with the same semantics (true if there is a single non-null value), since in this patch dd is count(distinct deptno), which is equivalent to d in the main branch.
so this when clause means "if there are any null values and there is a single non-null value".
in main, c = count(distinct *) and d = count(distinct deptno), which means that when there are null values and a single non-null value, c = d and this clause evaluates to false.
I know it's a tricky case, so let me know if additional clarifications are needed 😄
There was a problem hiding this comment.
Very well explained. A little problem
q.dd <= 1: is true if there is a single non-null value
should indicate that there is at most one non-null value. I think it can be true when there is no null value. WDYT
There was a problem hiding this comment.
oops, you're right, it means there is at most one non-null value.
if there's no null value, c should be equal to d, right? I can't think of a case with no null value that yield c <> d
There was a problem hiding this comment.
Yeah, that looks good.
There was a problem hiding this comment.
I mean, these are very nice comments.
There was a problem hiding this comment.
Looks good to me. The logic here is a bit complicated, you can add corresponding code comments @racevedoo.
There was a problem hiding this comment.
Just added the comments. IMO the whole context would be too long to fit in a comment, so I wrote a summary.
People can always refer to this PR to get a more complete understanding. WDYT?
There was a problem hiding this comment.
where is the summary you have written?
I think it's always best to have the code be self-explanatory.
Once someone makes any edit to this code finding the information residing in other tools like git will become harder.
There was a problem hiding this comment.
it's in the case statement comment, but I'll write the additional details.
racevedoo
force-pushed
the
fix-calcite-6778
branch
from
398e761 to
e201bfd
Compare
racevedoo
force-pushed
the
fix-calcite-6778
branch
from
e201bfd to
5b22225
Compare
mihaibudiu
added
the
LGTM-will-merge-soon
|
Can you please rebase this on main to resolve the conflicts? |
sure, I'll do it later today/tomorrow |
…ll values correctly
racevedoo
force-pushed
the
fix-calcite-6778
branch
from
5b22225 to
4b002e9
Compare
|
|
just rebased. also added the detailed comments |
No description provided.