tree: skip nulls when type checking a list of tuples #40298
Conversation
|
This change is |
|
I doubt that my WIP is the correct way to resolve the issue, but I don't know what is the expected behavior here, so any pointers/suggestions are very welcome. As a side note, Postgres doesn't allow empty tuples, why do we allow such case? |
|
Friendly ping. |
yuzefovich
changed the title
|
Friendly ping. I had a WIP in the title of the PR because I don't think that it is ready to be merged, but it is definitely ready for a look. I'd appreciate any input although it doesn't seem urgent. |
|
Are there new tests to add for this? It looks fine. |
yuzefovich
force-pushed
the
fix-tuple
branch
from
4f526b0
to
4bf2971
Compare
yuzefovich
changed the title
|
I expected that this approach would not be approved so didn't bother to add tests at first. I added a test which actually uncovered a bug in my initial implementation. PTAL. |
|
Does this PR also fix the following: And can you add a test? There's a couple other callers of TypeCheckSameTypedExprs like this - you don't need to test them all but adding another case would be a nice to have. Besides this LGTM. |
yuzefovich
force-pushed
the
fix-tuple
branch
from
4bf2971
to
28e8bee
Compare
|
Yes, the query you posted now works. I also tried a couple of others and found another edge case which my PR introduced, so I fixed that and added a few more logic tests. I'm not sure under which category for release justification this would go though. It's a bug fix to existing functionality, but the bug is not of high priority/severity. I feel like "Category 4: Low risk, high benefit changes to existing functionality" is the most suitable, but I would not say I'm confident in this change since I think it is possible that there are some other edge cases I'm introducing which can result in an internal error while the current master would return the type mismatch error. Should this go in for 19.2? |
|
I think it should be good to merge this now. |
|
Friendly ping. |
There was a problem hiding this comment.
LGTM but:
-
please remove the release justification line from the commit message
-
please add a release note
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
Previously, the type checking code for a list of tuples expected that all expressions in the list are actually tuples. Now we will also allow for nulls to be present in the list. Release note (sql change): NULLs are now allowed to be among tuples when being compared against a tuple.
|
TFTRs! bors r+ |
40298: tree: skip nulls when type checking a list of tuples r=yuzefovich a=yuzefovich Previously, the type checking code for a list of tuples expected that all expressions in the list are actually tuples. Now we will also allow for nulls to be present in the list. Fixes: #40297. Release note (sql change): NULLs are now allowed to be among tuples when being compared against a tuple. 41306: sql: add a clarifying comment about flow cleanup in distsql_running r=yuzefovich a=yuzefovich Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Build succeeded |
Previously, the type checking code for a list of tuples expected that
all expressions in the list are actually tuples. Now we will also allow
for nulls to be present in the list.
Fixes: #40297.
Release note (sql change): NULLs are now allowed to be among tuples when
being compared against a tuple.