Skip to content

Commit 0140089

Browse files
pitroupitrou
authored
GH-46791: [C++] Add Status::OrElse, IntoStatus<T> and ToStatus (#46792)
### Rationale for this change In #46711 (comment) it was mentioned that the macro `RETURN_NOT_OK_ELSE` is confusing and can easily be misunderstood. We would like a better way to conditionally chain error-handling code if a Status does not indicate success. ### What changes are included in this PR? 1. Add a type trait `IntoStatus<T>` that can be implemented to provide conversions from other error-like types 2. Add a global `ToStatus` function that calls the aforementioned type trait 3. Add a `Status::OrElse` method that calls a functor on error 4. Remove the `RETURN_NOT_OK_ELSE` macro ### Are these changes tested? Yes. ### Are there any user-facing changes? No, the `RETURN_NOT_OK_ELSE` was not supposed to be called by third-party code as it's not prefixed with `ARROW_`. * GitHub Issue: #46791 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <pitrou@free.fr>
1 parent 953947a commit 0140089

43 files changed

Lines changed: 323 additions & 222 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎c_glib/test/test-decimal128.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ def test_divide_zero
123123
decimal1 = Arrow::Decimal128.new(23423445)
124124
decimal2 = Arrow::Decimal128.new(0)
125125
message =
126-
"[decimal128][divide]: Invalid: Division by 0 in Decimal128"
126+
"[decimal128][divide]: Invalid: Division by 0 in Decimal"
127127
assert_raise(Arrow::Error::Invalid.new(message)) do
128128
decimal1.divide(decimal2)
129129
end
@@ -236,7 +236,7 @@ def test_rescale_fail
236236
decimal = Arrow::Decimal128.new(10)
237237
message =
238238
"[decimal128][rescale]: Invalid: " +
239-
"Rescaling Decimal128 value would cause data loss"
239+
"Rescaling Decimal value would cause data loss"
240240
assert_raise(Arrow::Error::Invalid.new(message)) do
241241
decimal.rescale(1, -1)
242242
end

‎c_glib/test/test-decimal256.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ def test_divide_zero
110110
decimal1 = Arrow::Decimal256.new(23423445)
111111
decimal2 = Arrow::Decimal256.new(0)
112112
message =
113-
"[decimal256][divide]: Invalid: Division by 0 in Decimal256"
113+
"[decimal256][divide]: Invalid: Division by 0 in Decimal"
114114
assert_raise(Arrow::Error::Invalid.new(message)) do
115115
decimal1.divide(decimal2)
116116
end
@@ -223,7 +223,7 @@ def test_rescale_fail
223223
decimal = Arrow::Decimal256.new(10)
224224
message =
225225
"[decimal256][rescale]: Invalid: " +
226-
"Rescaling Decimal256 value would cause data loss"
226+
"Rescaling Decimal value would cause data loss"
227227
assert_raise(Arrow::Error::Invalid.new(message)) do
228228
decimal.rescale(1, -1)
229229
end

‎c_glib/test/test-decimal32.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ def test_divide_zero
106106
decimal1 = Arrow::Decimal32.new(23423445)
107107
decimal2 = Arrow::Decimal32.new(0)
108108
message =
109-
"[decimal32][divide]: Invalid: Division by 0 in Decimal32"
109+
"[decimal32][divide]: Invalid: Division by 0 in Decimal"
110110
assert_raise(Arrow::Error::Invalid.new(message)) do
111111
decimal1.divide(decimal2)
112112
end
@@ -214,7 +214,7 @@ def test_rescale_fail
214214
decimal = Arrow::Decimal32.new(10)
215215
message =
216216
"[decimal32][rescale]: Invalid: " +
217-
"Rescaling Decimal32 value would cause data loss"
217+
"Rescaling Decimal value would cause data loss"
218218
assert_raise(Arrow::Error::Invalid.new(message)) do
219219
decimal.rescale(1, -1)
220220
end

‎c_glib/test/test-decimal64.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ def test_divide_zero
106106
decimal1 = Arrow::Decimal64.new(23423445)
107107
decimal2 = Arrow::Decimal64.new(0)
108108
message =
109-
"[decimal64][divide]: Invalid: Division by 0 in Decimal64"
109+
"[decimal64][divide]: Invalid: Division by 0 in Decimal"
110110
assert_raise(Arrow::Error::Invalid.new(message)) do
111111
decimal1.divide(decimal2)
112112
end
@@ -214,7 +214,7 @@ def test_rescale_fail
214214
decimal = Arrow::Decimal64.new(10)
215215
message =
216216
"[decimal64][rescale]: Invalid: " +
217-
"Rescaling Decimal64 value would cause data loss"
217+
"Rescaling Decimal value would cause data loss"
218218
assert_raise(Arrow::Error::Invalid.new(message)) do
219219
decimal.rescale(1, -1)
220220
end

‎cpp/src/arrow/array/concatenate_test.cc‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -430,14 +430,14 @@ TEST_F(ConcatenateTest, DictionaryTypeDifferentSizeIndex) {
430430
auto bigger_dict_type = dictionary(uint16(), utf8());
431431
auto dict_one = DictArrayFromJSON(dict_type, "[0]", "[\"A0\"]");
432432
auto dict_two = DictArrayFromJSON(bigger_dict_type, "[0]", "[\"B0\"]");
433-
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}).status());
433+
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}));
434434
}
435435

436436
TEST_F(ConcatenateTest, DictionaryTypeCantUnifyNullInDictionary) {
437437
auto dict_type = dictionary(uint8(), utf8());
438438
auto dict_one = DictArrayFromJSON(dict_type, "[0, 1]", "[null, \"A\"]");
439439
auto dict_two = DictArrayFromJSON(dict_type, "[0, 1]", "[null, \"B\"]");
440-
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}).status());
440+
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}));
441441
}
442442

443443
TEST_F(ConcatenateTest, DictionaryTypeEnlargedIndices) {
@@ -464,7 +464,7 @@ TEST_F(ConcatenateTest, DictionaryTypeEnlargedIndices) {
464464

465465
auto dict_one = std::make_shared<DictionaryArray>(dict_type, indices, dictionary_one);
466466
auto dict_two = std::make_shared<DictionaryArray>(dict_type, indices, dictionary_two);
467-
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}).status());
467+
ASSERT_RAISES(Invalid, Concatenate({dict_one, dict_two}));
468468

469469
auto bigger_dict_type = dictionary(uint16(), uint16());
470470

@@ -729,8 +729,7 @@ TEST_F(ConcatenateTest, OffsetOverflow) {
729729
fake_long_list->data()->child_data[0] = fake_long->data();
730730

731731
ASSERT_RAISES(Invalid, internal::Concatenate({fake_long_list, fake_long_list}, pool,
732-
&suggested_cast)
733-
.status());
732+
&suggested_cast));
734733
ASSERT_TRUE(suggested_cast->Equals(*expected_suggestion));
735734
}
736735
}
@@ -740,8 +739,7 @@ TEST_F(ConcatenateTest, OffsetOverflow) {
740739
fake_long_list->data()->GetMutableValues<int32_t>(1)[1] =
741740
std::numeric_limits<int32_t>::max();
742741
ASSERT_RAISES(Invalid, internal::Concatenate({fake_long_list, fake_long_list}, pool,
743-
&suggested_cast)
744-
.status());
742+
&suggested_cast));
745743
ASSERT_TRUE(suggested_cast->Equals(LargeVersionOfType(list_ty)));
746744

747745
auto list_view_ty = list_view(null());
@@ -757,8 +755,7 @@ TEST_F(ConcatenateTest, OffsetOverflow) {
757755
mutable_sizes[0] = kInt32Max;
758756
}
759757
ASSERT_RAISES(Invalid, internal::Concatenate({fake_long_list_view, fake_long_list_view},
760-
pool, &suggested_cast)
761-
.status());
758+
pool, &suggested_cast));
762759
ASSERT_TRUE(suggested_cast->Equals(LargeVersionOfType(list_view_ty)));
763760
}
764761

‎cpp/src/arrow/array/diff_test.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ class DiffTest : public ::testing::Test {
7676

7777
void DoDiff() {
7878
auto edits = Diff(*base_, *target_, default_memory_pool());
79-
ASSERT_OK(edits.status());
79+
ASSERT_OK(edits);
8080
edits_ = edits.ValueOrDie();
8181
ASSERT_OK(edits_->ValidateFull());
8282
ASSERT_TRUE(edits_->type()->Equals(edits_type));
@@ -87,7 +87,7 @@ class DiffTest : public ::testing::Test {
8787
void DoDiffAndFormat(std::stringstream* out) {
8888
DoDiff();
8989
auto formatter = MakeUnifiedDiffFormatter(*base_->type(), out);
90-
ASSERT_OK(formatter.status());
90+
ASSERT_OK(formatter);
9191
ASSERT_OK(formatter.ValueOrDie()(*edits_, *base_, *target_));
9292
}
9393

@@ -800,10 +800,10 @@ TEST_F(DiffTest, CompareRandomStruct) {
800800

801801
auto type = struct_({field("i", int32()), field("s", utf8())});
802802
auto base_res = StructArray::Make({int32_base, utf8_base}, type->fields());
803-
ASSERT_OK(base_res.status());
803+
ASSERT_OK(base_res);
804804
base_ = base_res.ValueOrDie();
805805
auto target_res = StructArray::Make({int32_target, utf8_target}, type->fields());
806-
ASSERT_OK(target_res.status());
806+
ASSERT_OK(target_res);
807807
target_ = target_res.ValueOrDie();
808808

809809
std::stringstream formatted;

‎cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ static void ArrayScalarKernel(benchmark::State& state) {
8080
rand.Numeric<ArrowType>(array_size, min, max, args.null_proportion));
8181

8282
for (auto _ : state) {
83-
ABORT_NOT_OK(Op(lhs, rhs, ArithmeticOptions(), nullptr).status());
83+
ABORT_NOT_OK(Op(lhs, rhs, ArithmeticOptions(), nullptr));
8484
}
8585
state.SetItemsProcessed(state.iterations() * array_size);
8686
}
@@ -103,7 +103,7 @@ static void ArrayArrayKernel(benchmark::State& state) {
103103
rand.Numeric<ArrowType>(array_size, rmin, rmax, args.null_proportion));
104104

105105
for (auto _ : state) {
106-
ABORT_NOT_OK(Op(lhs, rhs, ArithmeticOptions(), nullptr).status());
106+
ABORT_NOT_OK(Op(lhs, rhs, ArithmeticOptions(), nullptr));
107107
}
108108
state.SetItemsProcessed(state.iterations() * array_size);
109109
}

‎cpp/src/arrow/compute/kernels/scalar_boolean_benchmark.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ static void ArrayArrayKernel(benchmark::State& state) {
4242
auto rhs = rand.Boolean(array_size, /*true_probability=*/0.5, args.null_proportion);
4343

4444
for (auto _ : state) {
45-
ABORT_NOT_OK(Op(lhs, rhs, nullptr).status());
45+
ABORT_NOT_OK(Op(lhs, rhs, nullptr));
4646
}
4747
state.SetItemsProcessed(state.iterations() * array_size);
4848
}

‎cpp/src/arrow/compute/kernels/scalar_cast_benchmark.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ static void BenchmarkNumericCast(benchmark::State& state,
3737
random::RandomArrayGenerator rand(kSeed);
3838
auto array = rand.Numeric<InputType>(args.size, min, max, args.null_proportion);
3939
for (auto _ : state) {
40-
ABORT_NOT_OK(Cast(array, to_type, options).status());
40+
ABORT_NOT_OK(Cast(array, to_type, options));
4141
}
4242
}
4343

@@ -54,7 +54,7 @@ static void BenchmarkFloatingToIntegerCast(benchmark::State& state,
5454
std::shared_ptr<Array> values_as_float = *Cast(*array, from_type);
5555

5656
for (auto _ : state) {
57-
ABORT_NOT_OK(Cast(values_as_float, to_type, options).status());
57+
ABORT_NOT_OK(Cast(values_as_float, to_type, options));
5858
}
5959
}
6060

‎cpp/src/arrow/compute/kernels/scalar_compare_benchmark.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ static void BenchArrayScalar(benchmark::State& state, const std::string& op) {
3838
auto array = rand.ArrayOf(ty, args.size, args.null_proportion);
3939
auto scalar = *rand.ArrayOf(ty, 1, 0)->GetScalar(0);
4040
for (auto _ : state) {
41-
ABORT_NOT_OK(CallFunction(op, {array, Datum(scalar)}).status());
41+
ABORT_NOT_OK(CallFunction(op, {array, Datum(scalar)}));
4242
}
4343
}
4444

@@ -50,7 +50,7 @@ static void BenchArrayArray(benchmark::State& state, const std::string& op) {
5050
auto lhs = rand.ArrayOf(ty, args.size, args.null_proportion);
5151
auto rhs = rand.ArrayOf(ty, args.size, args.null_proportion);
5252
for (auto _ : state) {
53-
ABORT_NOT_OK(CallFunction(op, {lhs, rhs}).status());
53+
ABORT_NOT_OK(CallFunction(op, {lhs, rhs}));
5454
}
5555
}
5656

0 commit comments

Comments
 (0)