-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Kernel] Fixed potential float -> double casting issue and aligned IN expression with comparator "=" implementation #5298
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
Signed-off-by: isaiah mowbray <[email protected]>
|
|
||
| import org.scalatest.funsuite.AnyFunSuite | ||
|
|
||
| class ImplicitCastExpressionSuite extends AnyFunSuite with TestUtils { |
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.
Why do we want to remove this file?
| * class determines the narrowest common type that all operands can be cast to, following the same | ||
| * type precedence hierarchy as {@link ImplicitCastExpression}. | ||
| */ | ||
| final class TypeResolver { |
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.
Can we try to move the utils defined in
Lines 102 to 126 in 73af97a
| private static final Map<String, List<String>> UP_CASTABLE_TYPE_TABLE = | |
| unmodifiableMap( | |
| new HashMap<String, List<String>>() { | |
| { | |
| this.put("byte", Arrays.asList("short", "integer", "long", "float", "double")); | |
| this.put("short", Arrays.asList("integer", "long", "float", "double")); | |
| this.put("integer", Arrays.asList("long", "float", "double")); | |
| this.put("long", Arrays.asList("float", "double")); | |
| this.put("float", Arrays.asList("double")); | |
| } | |
| }); | |
| /** | |
| * Utility method which returns whether the given {@code from} type can be cast to {@code to} | |
| * type. | |
| */ | |
| static boolean canCastTo(DataType from, DataType to) { | |
| // TODO: The type name should be a first class method on `DataType` instead of getting it | |
| // using the `toString`. | |
| String fromStr = from.toString(); | |
| String toStr = to.toString(); | |
| return UP_CASTABLE_TYPE_TABLE.containsKey(fromStr) | |
| && UP_CASTABLE_TYPE_TABLE.get(fromStr).contains(toStr); | |
| } | |
This method is good but want to see if we could just re-use code
| List<Expression> transformedList = | ||
| applyListCasts(inListExpressions, inListDataTypes, commonType); | ||
|
|
||
| validateCollation( |
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.
Let's do the collation validation first, casting to common type is our implementation that may not be related to the user provided expression
Which Delta project/connector is this regarding?
Description
Resolves #5227
Investigated float-to-double precision issue in ImplicitCastExpression
Refactor validateAndTransform method to use implicit casting
Simplify value comparison logic in InColumnVector
How was this patch tested?
New UTs and validated existing UTs
Does this PR introduce any user-facing changes?
No