GH-39051: [C++] Use Cast() instead of CastTo() for List Scalar in test #39353
Conversation
|
|
cpp/src/arrow/scalar_test.cc
Outdated
| void CheckInvalidListCast(const ScalarType& scalar, | ||
| const std::shared_ptr<DataType>& to_type, const StatusCode code, | ||
| const std::string& expected_message) { | ||
| EXPECT_RAISES_WITH_CODE_AND_MESSAGE_THAT(StatusCode::Invalid, | ||
| ::testing::HasSubstr(expected_message), | ||
| scalar.CastTo(to_type)); | ||
| EXPECT_RAISES_WITH_CODE_AND_MESSAGE_THAT(code, ::testing::HasSubstr(expected_message), |
There was a problem hiding this comment.
It's strange that CheckInvalidListCast() may not expect StatusCode::Invalid.
Can we use better function name?
There was a problem hiding this comment.
Yes. You are correct. In the current situation, as you mentioned, both Invalid and TypeError can occur. I think CheckListCastError or VerifyListCastFailure would be more appropriate. What do you think? (First, I chose CheckListCastError.)
There was a problem hiding this comment.
@kou Hello. When you have free time, could you please review it? Thank you :)
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 8a9f877. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Remove legacy code
What changes are included in this PR?
Replace the legacy scalar CastTo implementation for List Scalar in test.
Are these changes tested?
Yes. It is passed by existing test cases.
Are there any user-facing changes?
No.