Re-enable comparison type match check in DomainTranslator#13561
Conversation
findepi
added
the
no-release-notes
findepi
force-pushed
the
findepi/re-enable-comparison-type-match-check-in-domaintranslator-a32c44
branch
from
e815f3c to
1591d93
Compare
Plan tests should take well-formed plan nodes as input. In plan nodes, the contained expressions should have all correct types (should not require any coercions, which are attribute of the SQL language and its processing, do not belong to plan nodes' world).
It was temporarily disabled in March 2017 in commit a103a63.
findepi
force-pushed
the
findepi/re-enable-comparison-type-match-check-in-domaintranslator-a32c44
branch
from
1591d93 to
025ccc2
Compare
| Type leftType = expressionTypes.get(NodeRef.of(comparison.getLeft())); | ||
| Type rightType = expressionTypes.get(NodeRef.of(comparison.getRight())); | ||
|
|
||
| // TODO: re-enable this check once we fix the type coercions in the optimizers |
There was a problem hiding this comment.
This PR fixes some tests. I don't know whether there were any other fixes related to this.
There was a problem hiding this comment.
Sure, but this change is not in tests, but in product
There was a problem hiding this comment.
Agreed. The check is sound. The tests say it should be fine. What else can I do?
There was a problem hiding this comment.
Why was check disabled in the first place and what fixed it so that we can enable it again? Was it only tests?
There was a problem hiding this comment.
The check was introduced by @losipiuk in 03f5942
and disabled by @erichwang in a103a63.
Eric made it clear that the check is disabled temporarily.
The check is sound and important for reasoning about optimizers, we rely on comparison's sides being same type in many places. We shouldn't be afraid of enabling the check here.
There was a problem hiding this comment.
Sure, but what changed so that a103a63 is no longer relevant?
There was a problem hiding this comment.
it was over 5 years ago. i think it will be hard to find out.
There was a problem hiding this comment.
Does it have potential to cause regressions?
There was a problem hiding this comment.
Absolutely! (as pretty much any other change in the production code base)
|
@sopel39 PTAL |
3 participants
It was temporarily disabled in March 2017 in commit
a103a63.