kou commented on code in PR #39353:
URL: https://github.com/apache/arrow/pull/39353#discussion_r1438879932
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1178,10 +1196,10 @@ class TestListLikeScalar : public ::testing::Test {
CheckListCast(
scalar, fixed_size_list(value_->type(),
static_cast<int32_t>(value_->length())));
- CheckInvalidListCast(scalar, fixed_size_list(value_->type(), 5),
- "Cannot cast " + scalar.type->ToString() + " of
length " +
- std::to_string(value_->length()) +
- " to fixed size list of length 5");
+ auto invalidCastType = fixed_size_list(value_->type(), 5);
Review Comment:
Could you use `snake_case`?
```suggestion
auto invalid_cast_type = fixed_size_list(value_->type(), 5);
```
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1238,10 +1256,10 @@ TEST(TestMapScalar, Cast) {
CheckListCast(scalar, large_list(key_value_type));
CheckListCast(scalar, fixed_size_list(key_value_type, 2));
- CheckInvalidListCast(scalar, fixed_size_list(key_value_type, 5),
- "Cannot cast " + scalar.type->ToString() + " of length
" +
- std::to_string(value->length()) +
- " to fixed size list of length 5");
+ auto invalidCastType = fixed_size_list(key_value_type, 5);
Review Comment:
ditto.
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1087,11 +1088,28 @@ void CheckListCast(const ScalarType& scalar, const
std::shared_ptr<DataType>& to
*checked_cast<const
BaseListScalar&>(*cast_scalar).value);
}
-void CheckInvalidListCast(const Scalar& scalar, const
std::shared_ptr<DataType>& to_type,
- const std::string& expected_message) {
- EXPECT_RAISES_WITH_CODE_AND_MESSAGE_THAT(StatusCode::Invalid,
-
::testing::HasSubstr(expected_message),
- scalar.CastTo(to_type));
+std::tuple<StatusCode, std::string> GetExpectedError(
+ const std::shared_ptr<DataType>& type,
+ const std::shared_ptr<DataType>& invalidCastType) {
+ if (type->id() == Type::FIXED_SIZE_LIST) {
+ return std::make_tuple(
+ StatusCode::TypeError,
+ "Size of FixedSizeList is not the same. input list: " +
type->ToString() +
+ " output list: " + invalidCastType->ToString());
+ } else {
+ return std::make_tuple(
+ StatusCode::Invalid,
+ "ListType can only be casted to FixedSizeListType if the lists are all
the "
+ "expected size.");
+ }
+}
Review Comment:
Both of them just pass the returned value of `GetExpectedError()` to
`CheckListCastError()`.
I think that we don't need to split `GetExpectedError()` and
`CheckListCastError()` (I think that we don't have code duplication without
splitting to them):
```diff
diff --git a/cpp/src/arrow/scalar_test.cc b/cpp/src/arrow/scalar_test.cc
index 3170a00e34..fefa8b8b52 100644
--- a/cpp/src/arrow/scalar_test.cc
+++ b/cpp/src/arrow/scalar_test.cc
@@ -1088,26 +1088,23 @@ void CheckListCast(const ScalarType& scalar, const
std::shared_ptr<DataType>& to
*checked_cast<const
BaseListScalar&>(*cast_scalar).value);
}
-std::tuple<StatusCode, std::string> GetExpectedError(
- const std::shared_ptr<DataType>& type,
- const std::shared_ptr<DataType>& invalidCastType) {
- if (type->id() == Type::FIXED_SIZE_LIST) {
- return std::make_tuple(
- StatusCode::TypeError,
- "Size of FixedSizeList is not the same. input list: " +
type->ToString() +
- " output list: " + invalidCastType->ToString());
- } else {
- return std::make_tuple(
- StatusCode::Invalid,
- "ListType can only be casted to FixedSizeListType if the lists are
all the "
- "expected size.");
- }
-}
-
template <typename ScalarType>
void CheckListCastError(const ScalarType& scalar,
- const std::shared_ptr<DataType>& to_type, const
StatusCode code,
- const std::string& expected_message) {
+ const std::shared_ptr<DataType>& from_type,
+ const std::shared_ptr<DataType>& to_type) {
+ StatusCode code;
+ std::string expected_message;
+ if (from_type->id() == Type::FIXED_SIZE_LIST) {
+ code = StatusCode::TypeError;
+ expected_message =
+ "Size of FixedSizeList is not the same. input list: " +
from_type->ToString() +
+ " output list: " + to_type->ToString();
+ } else {
+ code = StatusCode::Invalid;
+ expected_message =
+ "ListType can only be casted to FixedSizeListType if the lists are
all the "
+ "expected size.";
+ }
EXPECT_RAISES_WITH_CODE_AND_MESSAGE_THAT(code,
::testing::HasSubstr(expected_message),
Cast(scalar, to_type));
}
@@ -1197,9 +1194,7 @@ class TestListLikeScalar : public ::testing::Test {
scalar, fixed_size_list(value_->type(),
static_cast<int32_t>(value_->length())));
auto invalidCastType = fixed_size_list(value_->type(), 5);
- auto [expectedCode, expectedMessage] = GetExpectedError(type_,
invalidCastType);
-
- CheckListCastError(scalar, invalidCastType, expectedCode,
expectedMessage);
+ CheckListCastError(scalar, type_, invalidCastType);
}
protected:
@@ -1257,9 +1252,7 @@ TEST(TestMapScalar, Cast) {
CheckListCast(scalar, fixed_size_list(key_value_type, 2));
auto invalidCastType = fixed_size_list(key_value_type, 5);
- auto [expectedCode, expectedMessage] = GetExpectedError(scalar.type,
invalidCastType);
-
- CheckListCastError(scalar, invalidCastType, expectedCode,
expectedMessage);
+ CheckListCastError(scalar, scalar.type, invalidCastType);
}
TEST(TestStructScalar, FieldAccess) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
