Uh oh!
There was an error while loading. Please reload this page.
ARROW-4957: [Rust] [DataFusion] Re-implement get_supertype - #4019
ARROW-4957: [Rust] [DataFusion] Re-implement get_supertype #4019andygrove wants to merge 2 commits into
Conversation
andygrove
commented
Mar 23, 2019
@kszucs Please take a look when you can |
andygrove
commented
Mar 25, 2019
@paddyhoran@sunchao Some code cleanup that would be good to get into 0.13 |
sunchao
left a comment
There was a problem hiding this comment.
Thanks @andygrove . Sorry for the late review.
| ))), | ||
| }, | ||
| use arrow::datatypes::DataType::*; | ||
| let d = if l == r { |
There was a problem hiding this comment.
In the old implementation the super type of UInt16 and Int8 is None but now it is Int16. Why is that? Also do you know how C++ handles this case?
There was a problem hiding this comment.
It is not statically compatible in C++ (safely). https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/cast.cc#L156-L208
It can be safe to cast, but it needs a runtime check.
| )); | ||
| } | ||
| } | ||
| assert_eq!( |
There was a problem hiding this comment.
Can we avoid comparing with string? whenever there's an error it is very hard to find out since you are comparing two huge strings.
| ))), | ||
| }, | ||
| use arrow::datatypes::DataType::*; | ||
| let d = if l == r { |
There was a problem hiding this comment.
It is not statically compatible in C++ (safely). https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/cast.cc#L156-L208
It can be safe to cast, but it needs a runtime check.
| Some(l.clone()) | ||
| } else if l == &Utf8 || r == &Utf8 { | ||
| Some(Utf8) | ||
| } else if is_signed_int(l) { |
There was a problem hiding this comment.
This block is hard to follow, I'd rewrite this with small functions.
wesm
commented
May 23, 2019
Can this PR be merged? |
sunchao
commented
May 24, 2019
I think there are a few pending comments to be resolved. @andygrove can you take a look? |
andygrove
commented
May 25, 2019
Sorry, I haven't had any spare time for a while due to work commitments. I am closing this and will try again another time. |
This PR re-implements get_supertype to be easier to read and maintain. It still only supports numeric types and string. After this PR is merged I will create new PRs to add rules for boolean and date/time.