-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-18481: [C++] prefer casting literal over casting field ref #15180
GH-18481: [C++] prefer casting literal over casting field ref #15180
Conversation
|
@@ -368,6 +368,153 @@ bool Expression::IsSatisfiable() const { | |||
|
|||
namespace { | |||
|
|||
TypeHolder SmallestTypeFor(const arrow::Datum& value) { |
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.
I feel like this is a rather naive approach but it seems to work.
} | ||
case Type::DOUBLE: { | ||
double doub = value.scalar_as<DoubleScalar>().value; | ||
if (double(float(doub)) == doub) { |
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.
Is this the best way to determine if a double can be exactly represented by a float?
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 doesn't catch nan
s. I'd recommend including an explicit clause for nan
s and inf
s. For the value itself, though, I think this roundtrip is reasonable.
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.
What do you think of checking fmod(doub) == 0
and demoting to integer? This would simplify expressions like i32 > 0.0
to an integer comparison. Perhaps not worth it?
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.
At the moment I'm thinking no for floating->integral. In general it should be easy enough to convert from one literal to another in just about any serialization of expressions (e.g. change 1.0 to 1).
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.
I've added a check for nan
/inf
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.
I think the general approach is acceptable, modulo a few nits.
For future reference, my own plan for this issue was to write this as a simplification pass. This approach is much simpler and only introduces a few minor edge cases. Nicely done
} | ||
case Type::DOUBLE: { | ||
double doub = value.scalar_as<DoubleScalar>().value; | ||
if (double(float(doub)) == doub) { |
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 doesn't catch nan
s. I'd recommend including an explicit clause for nan
s and inf
s. For the value itself, though, I think this roundtrip is reasonable.
} | ||
case Type::DOUBLE: { | ||
double doub = value.scalar_as<DoubleScalar>().value; | ||
if (double(float(doub)) == doub) { |
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.
What do you think of checking fmod(doub) == 0
and demoting to integer? This would simplify expressions like i32 > 0.0
to an integer comparison. Perhaps not worth it?
ExpectBindsTo(cmp(field_ref("dict_i32"), literal(int64_t(4))), | ||
cmp(cast(field_ref("dict_i32"), int64()), literal(int64_t(4)))); | ||
cmp(field_ref("dict_i32"), literal(int32_t(4)))); | ||
ExpectBindsTo(cmp(field_ref("dict_i32"), literal(int64_t(4))), |
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.
Please add a case which demonstrates the behavior in the presence of unsigned integers. SmallestTypeFor
preserves signedness information, which can produce some odd edge cases which we should be explicit about. For example:
ExpectBindsTo(cmp(field_ref("dict_i32"), literal(int64_t(4))), | |
ExpectBindsTo(cmp(field_ref("i8"), literal(uint8_t(4))), | |
cmp(cast(field_ref("i8"), int16()), literal(int16_t(4)))); | |
ExpectBindsTo(cmp(field_ref("dict_i32"), literal(int64_t(4))), |
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.
Done.
…that literal(NaN) == literal(NaN)
2870fb2
to
cadf4ae
Compare
…32 instead of float64. Fix test bug in R
The test failures revealed another issue which I addressed here. In @nealrichardson there was another R test failure which I deemed inevitable and I have updated the test. It is a slight behavior change. Does this seem acceptable: https://github.com/apache/arrow/pull/15180/files#diff-45391fbe156c77f99e14090369faed8e11ec49583634fa1a51b634ef8b9eb5bf |
Let me take a look. If this PR does what I think it's doing, there's probably a bunch of R code I can delete now that attempted the same. |
float64 sounds fine to me (I'm surprised it was float32 actually) |
@@ -280,7 +280,9 @@ test_that("infer_type() gets the right type for Expression", { | |||
expect_equal(y$type(), infer_type(y)) | |||
expect_equal(infer_type(y), float64()) | |||
expect_equal(add_xy$type(), infer_type(add_xy)) | |||
expect_equal(infer_type(add_xy), float64()) | |||
# even though 10 is a float64, arrow will clamp it to the narrowest |
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 is only because both are scalars? If the float64 were corresponding to a field in the data, I would think that downcasting to float32 would be undesirable.
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.
Correct.
It would be good to run the macrobenchmarks on this. I would expect some speedup, mainly in the python queries (assuming we have any that would trigger this) since much of this is already handled in the R code, because we should be avoiding some big casts. Perhaps more importantly though, I ran into some issues with decimal types in #14553 and had to exclude decimals from the type conversion. I noticed this because a TPC-H query errored as a result of trying to keep operations on decimals. |
I will but I'm not certain we will get any speedup.
We don't. The python queries are extremely basic.
Yes. In fact, for some of the simpler TPC-H queries, that conversion from decimal to double is a majority of the execution time. Unfortunately, this PR does not fix that particular case because we don't support decimal arithmetic so a cast is inevitable. The implementation is a bit subtle. All of our non-decimal implicit casts go from "narrow type" to "wider type" (e.g. it is a safe cast). For example, if we have With decimal implicit casts we actually go in the opposite direction. We go from decimal (a wide type) to float64 (a more narrow type). Since we don't shrink a float64 literal into a decimal literal we still end up casting the array and not the literal. |
@ursabot please benchmark lang=R |
Benchmark runs are scheduled for baseline = 7eeb74e and contender = c8250e3. Results will be available as each benchmark for each run completes. |
|
Benchmark runs are scheduled for baseline = 838d0da and contender = b56b91e. b56b91e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
I pulled this, removed the R code that tries to make scalar inputs match the type of the corresponding fields where safe and appropriate, and ran the tests. I was surprised to see that |
@bkietz had suggested a similar rule. I am a little bit unsure about switching over types like that but we can add it if we want.
I found this surprising as well (I added a test case for it to draw attention to it). I think it is the correct thing to do though (although I may be biased as I think avoiding this situation would end up being tricky). Plus, if a user wants, they can always explicitly cast to get their desired behavior. Also, even if we adopted the above rule, this scenario could still occur with something like Yes, you have the negative consequence that the result of that 1.5x might not be easily represented in |
…pache#15180) I ran into this problem while trying to work out partition pruning in the new scan node. I feel like this is a somewhat naive approach but it seems to work. I think it would fail if a `DispatchBest` existed where a n-ary kernel existed with non-equal types. For example, if there was a function foo(int8, int32) and it had a dispatch best of some kind. Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>
…pache#15180) I ran into this problem while trying to work out partition pruning in the new scan node. I feel like this is a somewhat naive approach but it seems to work. I think it would fail if a `DispatchBest` existed where a n-ary kernel existed with non-equal types. For example, if there was a function foo(int8, int32) and it had a dispatch best of some kind. Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>
…pache#15180) I ran into this problem while trying to work out partition pruning in the new scan node. I feel like this is a somewhat naive approach but it seems to work. I think it would fail if a `DispatchBest` existed where a n-ary kernel existed with non-equal types. For example, if there was a function foo(int8, int32) and it had a dispatch best of some kind. Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>
### Rationale for this change This patch ( #15180 ) adds a `SmallestTypeFor` to handling expression type. However, it lost timezone when handling. ### What changes are included in this PR? Add `timezone` in `SmallestTypeFor` ### Are these changes tested? Currently not ### Are there any user-facing changes? Yeah it's a bugfix * Closes: #37110 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
…pache#37135) ### Rationale for this change This patch ( apache#15180 ) adds a `SmallestTypeFor` to handling expression type. However, it lost timezone when handling. ### What changes are included in this PR? Add `timezone` in `SmallestTypeFor` ### Are these changes tested? Currently not ### Are there any user-facing changes? Yeah it's a bugfix * Closes: apache#37110 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
I ran into this problem while trying to work out partition pruning in the new scan node. I feel like this is a somewhat naive approach but it seems to work.
I think it would fail if a
DispatchBest
existed where a n-ary kernel existed with non-equal types. For example, if there was a function foo(int8, int32) and it had a dispatch best of some kind.