[BEAM-9670] Fix nullability widening in CoGroup key resolution #11290
Conversation
|
run sql postcommit |
|
Run Java PreCommit |
| + leftSchema | ||
| + " rhsSchema: " | ||
| + rightSchema); | ||
| } |
There was a problem hiding this comment.
Shouldn't we still verify that the schemas have equivalent types without nullability?
There was a problem hiding this comment.
The merge function itself now verifies this.
There was a problem hiding this comment.
Ah I see. If there aren't any tests verifying this already could you add one?
There was a problem hiding this comment.
There is a test in CoGroupTest.java that verifies this. Should I add an additional one here?
There was a problem hiding this comment.
It could be nice to have something in ZetaSQLDialectSpecTest to verify the set operation just in case we break the logic there and not in CoGroup, but I guess it's fine either way
| default: | ||
| result = fieldType1; | ||
| } | ||
| return result.withNullable(fieldType1.getNullable() || fieldType2.getNullable()); |
There was a problem hiding this comment.
Just a general comment: do you think we should add a visitor pattern for this sort of thing?
There was a problem hiding this comment.
Maybe? I'm not sure visitor per se, as we don't require double dispatch. However we might consider generalizing the recursive tree walk.
|
@TheNeuralBit thanks. I'll merge once tests complete |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
This PR also adds some zetasql unit tests that expose the previous bug.