-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
evalengine: fix numeric coercibility #14473
Conversation
Signed-off-by: Vicent Marti <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Vicent Marti <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>
@@ -68,6 +68,7 @@ func createUncertain(direct TableSet, recursive TableSet) *uncertain { | |||
dependency: dependency{ | |||
direct: direct, | |||
recursive: recursive, | |||
typ: evalengine.UnknownType(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not being initialized as Unknown, so it was propagating as NULL
.
@@ -58,11 +58,10 @@ func (t *typer) up(cursor *sqlparser.Cursor) error { | |||
if !ok { | |||
return nil | |||
} | |||
var inputType sqltypes.Type | |||
inputType := sqltypes.Unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This was not being initialized as Unknown.
Signed-off-by: Vicent Marti <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
06d2d5e
to
492ca0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fuzzer to find that we're not doing this right before and using the wrong coercions!
Description
@systay discovered this issue during fuzzing. We need special handling for the coercibility of numeric types in the
evalengine
, otherwise somecase
expressions will not be collated as text as you'd expect.We've fixed the issue by replacing the generic
defaultCoercionCollation
helper with an enhanced version that takes into account the type of the value being collated. This also fixes mismatched collations for some datetime and JSON values.Here's an example of an expression that had the right return type, but the wrong collation before this change:
cc @dbussink
Related Issue(s)
Fixes #14480
Checklist
Deployment Notes