Add CAST (double as decimal) #8500
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
rui-mo
changed the title
rui-mo
force-pushed
the
wip_double
branch
3 times, most recently
from
50f3119
to
5394aa0
Compare
There was a problem hiding this comment.
@mbasmanova Could you help review this PR? Some unmerged changes from #8478 are added in this PR if still needed. Thanks.
| @@ -54,6 +54,13 @@ | |||
| VELOX_ASSERT_THROW_IMPL( \ | |||
| facebook::velox::VeloxRuntimeError, _expression, _errorMessage) | |||
|
|
|||
| #define VELOX_ASSERT_ERROR_STATUS(_expression, _statusCode, _errorMessage) \ | |||
There was a problem hiding this comment.
Add VELOX_ASSERT_ERROR_STATUS if it is needed.
| testComplexCast( | ||
| "c0", | ||
| makeConstant<double>(std::numeric_limits<double>::min(), 1), | ||
| makeConstant<int128_t>(0, 1, DECIMAL(38, 2))); |
There was a problem hiding this comment.
Tests for both lowest and min are added.
velox/common/base/tests/GTestUtils.h
Outdated
| @@ -54,6 +54,13 @@ | |||
| VELOX_ASSERT_THROW_IMPL( \ | |||
| facebook::velox::VeloxRuntimeError, _expression, _errorMessage) | |||
|
|
|||
| #define VELOX_ASSERT_ERROR_STATUS(_expression, _statusCode, _errorMessage) \ | |||
| const auto status = (_expression); \ | |||
| ASSERT_TRUE(status.code() == _statusCode); \ | |||
There was a problem hiding this comment.
Would it be possible to include mismatches codes in the error message? What does the failure look like now?
There was a problem hiding this comment.
Added code in the error message. Now the message looks like below.
Code mismatch:
Expected error code to be 'Type error', but received 'User error'.
Message mismatch:
Value of: status.message().find(expectedErrorMessage) != std::string::npos
Actual: false
Expected: true
Expected error message to contain 'Unknown error.', but received 'Result overflows.'.
velox/expression/CastExpr-inl.h
Outdated
| const auto toPrecisionScale = getDecimalPrecisionScale(*toType); | ||
|
|
||
| applyToSelectedNoThrowLocal(context, rows, result, [&](vector_size_t row) { | ||
| if (sourceVector->isNullAt(row)) { |
There was a problem hiding this comment.
Will this ever be the case? Are we not handling nulls elsewhere?
There was a problem hiding this comment.
I checked and found this branch would not be entered. Removed this branch and I will open a PR to remove similar checks of other cast kernels.
There was a problem hiding this comment.
Opened #8557 for this minor fix. Thanks.
| DECIMAL(10, 4))); | ||
|
|
||
| // Double to long decimal. | ||
| testComplexCast( |
There was a problem hiding this comment.
Can you remind me what does 'complex' mean here? This is not about complex types, is it?
There was a problem hiding this comment.
It is similar with testCast with some tiny differences. This is one PR #8254 to combine them inspired by #7377 (comment). cc @kagamiori
There was a problem hiding this comment.
Got it. Thank you for clarifying. Let's review and land that other PR to reduce confusion.
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova merged this pull request in 3596a72. |
|
Conbench analyzed the 1 benchmark run on commit There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
A replacement for #5767 from @wuxueyang96.
#8412