You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/08/24 16:14:03 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
lidavidm opened a new pull request #10987:
URL: https://github.com/apache/arrow/pull/10987
This removes fill_null in favor of coalesce.
A few points:
- This leaves behind the Python aliases for convenience.
- This is about 25% slower than fill_null for the (Array, Scalar) case for fixed-width types. As far as I can see, this is mostly because the handwritten fill_null only unboxes the scalar once (and does so cheaply), while the generic implementation here unboxes the scalar on every loop (the compiler doesn't manage to hoist it) *and* UnboxScalar is relatively expensive (requiring a virtual call). I tried to make UnboxScalar less expensive but couldn't avoid undefined behavior/incorrect results for the DayTimeIntervalType case.
- This specializes all the two-argument cases for fixed-width types but it might not really be worth it except for the (Array, Scalar) case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906612798
Benchmark runs are scheduled for baseline = 20885df121266a8519692c7630c98beb570f3b47 and contender = 43a44500991e2193a99e582d50f669ab1f6beeea. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/b0b82bffb57d4625b61eeb1d913c6373...dff104776e154921b3494fb1709d8eaf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7e6b3303b2c4441bab00c2f4aacd238d...e6aca9e032be4c0caa030c0dfa726d69/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5d7f13b26a47454f9337081a24b98b6e...47a8ef6a5e2e4569bcde6bc342177410/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r696696745
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
Thanks for the numbers!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r696610595
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
Just for completeness, I tested 50% and 99% nulls too. BitRunReader is still faster overall, maybe a bit slower with 50% nulls with strings.
```
--------------------------------------------------------------------------------------
Benchmark Time CPU Iterations UserCounters...
--------------------------------------------------------------------------------------
BitBlockCounter:
CoalesceScalarBench64/0 1488163 ns 1488088 ns 457 bytes_per_second=5.25003G/s items_per_second=704.647M/s length=1048.58k null%=1 num_args=2
CoalesceScalarBench64/2 5307719 ns 5307520 ns 133 bytes_per_second=1.47197G/s items_per_second=197.564M/s length=1048.58k null%=25 num_args=2
CoalesceScalarBench64/4 8338070 ns 8337713 ns 83 bytes_per_second=959.496M/s items_per_second=125.763M/s length=1048.58k null%=50 num_args=2
CoalesceScalarBench64/6 2187502 ns 2187409 ns 323 bytes_per_second=3.57158G/s items_per_second=479.369M/s length=1048.58k null%=99 num_args=2
CoalesceScalarStringBench/0 112844773 ns 112838638 ns 5 bytes_per_second=4.41803G/s items_per_second=9.2927M/s length=1048.58k null%=1 num_args=2
CoalesceScalarStringBench/2 94380710 ns 94376399 ns 8 bytes_per_second=4.01386G/s items_per_second=11.1106M/s length=1048.58k null%=25 num_args=2
CoalesceScalarStringBench/4 54766774 ns 54766616 ns 10 bytes_per_second=4.64332G/s items_per_second=19.1463M/s length=1048.58k null%=50 num_args=2
CoalesceScalarStringBench/6 6268787 ns 6268586 ns 108 bytes_per_second=1.41205G/s items_per_second=167.275M/s length=1048.58k null%=99 num_args=2
BitRunReader:
CoalesceScalarBench64/0 994000 ns 994007 ns 717 bytes_per_second=7.8596G/s items_per_second=1054.9M/s length=1048.58k null%=1 num_args=2
CoalesceScalarBench64/2 4624991 ns 4625012 ns 153 bytes_per_second=1.68918G/s items_per_second=226.719M/s length=1048.58k null%=25 num_args=2
CoalesceScalarBench64/4 7016190 ns 7016189 ns 102 bytes_per_second=1.1135G/s items_per_second=149.451M/s length=1048.58k null%=50 num_args=2
CoalesceScalarBench64/6 1013352 ns 1013358 ns 672 bytes_per_second=7.70952G/s items_per_second=1034.75M/s length=1048.58k null%=99 num_args=2
CoalesceScalarStringBench/0 110433694 ns 110433042 ns 5 bytes_per_second=4.51427G/s items_per_second=9.49513M/s length=1048.58k null%=1 num_args=2
CoalesceScalarStringBench/2 79904343 ns 79904642 ns 7 bytes_per_second=4.74082G/s items_per_second=13.1228M/s length=1048.58k null%=25 num_args=2
CoalesceScalarStringBench/4 61812967 ns 61813273 ns 10 bytes_per_second=4.11398G/s items_per_second=16.9636M/s length=1048.58k null%=50 num_args=2
CoalesceScalarStringBench/6 6219633 ns 6219571 ns 105 bytes_per_second=1.42317G/s items_per_second=168.593M/s length=1048.58k null%=99 num_args=2
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dianaclarke commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
dianaclarke commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906612083
@ursabot please benchmark
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906586422
I've submitted a PR to your branch to improve performance:
https://github.com/lidavidm/arrow/pull/4
I think the same approach may be used for other conditional kernels, but that would be a separate JIRA.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906612798
Benchmark runs are scheduled for baseline = 20885df121266a8519692c7630c98beb570f3b47 and contender = 43a44500991e2193a99e582d50f669ab1f6beeea. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/b0b82bffb57d4625b61eeb1d913c6373...dff104776e154921b3494fb1709d8eaf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7e6b3303b2c4441bab00c2f4aacd238d...e6aca9e032be4c0caa030c0dfa726d69/)
[Finished :arrow_down:0.55% :arrow_up:0.51%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5d7f13b26a47454f9337081a24b98b6e...47a8ef6a5e2e4569bcde6bc342177410/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r695841734
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
Have you tried using `BitRunReader` instead?
(er, perhaps we already had this conversation? :-S)
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
+ left_arr.length);
+ int64_t offset = 0;
+
+ const uint8_t* left_values = left_arr.buffers[1]->data();
+ const Scalar& right_scalar = *right.scalar();
+ while (offset < length) {
+ const auto block = bit_counter.NextWord();
+ if (block.AllSet()) {
+ // All from left
+ CopyFixedWidth<Type>::CopyArray(*left_arr.type, left_values,
+ left_arr.offset + offset, block.length, out_values,
+ out_offset + offset);
+ } else if (block.NoneSet()) {
+ // All from right
+ CopyFixedWidth<Type>::CopyScalar(right_scalar, block.length, out_values,
+ out_offset + offset);
+ } else if (block.popcount) {
+ // One by one
+ for (int64_t j = 0; j < block.length; ++j) {
+ if (BitUtil::GetBit(left_valid, left_arr.offset + offset + j)) {
+ CopyFixedWidth<Type>::CopyArray(
+ *left_arr.type, left_values, left_arr.offset + offset + j,
+ /*length=*/1, out_values, out_offset + offset + j);
+ } else {
+ CopyFixedWidth<Type>::CopyScalar(right_scalar, /*length=*/1, out_values,
+ out_offset + offset + j);
+ }
+ }
+ }
+ offset += block.length;
+ }
+
+ if (right_scalar.is_valid || !left_valid) {
+ BitUtil::SetBitsTo(out_valid, out_offset, length, true);
+ } else {
+ arrow::internal::CopyBitmap(left_valid, left_arr.offset, length, out_valid,
+ out_offset);
+ }
+ return Status::OK();
+}
+
+// Special case: implement 'coalesce' for any 2 arguments for any fixed-width
+// type (a 'fill_null' operation)
+template <typename Type>
+Status ExecBinaryCoalesce(KernelContext* ctx, Datum left, Datum right, int64_t length,
+ Datum* out) {
+ if (left.is_scalar() && right.is_scalar()) {
+ // Both scalar
+ *out = left.scalar()->is_valid ? left : right;
+ return Status::OK();
+ }
+
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ if (left.is_scalar()) {
+ // LHS is scalar
+ CopyValues<Type>(left.scalar()->is_valid ? left : right, /*in_offset=*/0, length,
+ out_valid, out_values, out_offset);
+ return Status::OK();
+ }
+
+ // Array with nulls
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
+ left_arr.length);
+ int64_t offset = 0;
+
+ if (right.is_scalar()) {
Review comment:
Shouldn't this be moved up before `// Array with nulls`?
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
+ left_arr.length);
+ int64_t offset = 0;
+
+ const uint8_t* left_values = left_arr.buffers[1]->data();
+ const Scalar& right_scalar = *right.scalar();
+ while (offset < length) {
+ const auto block = bit_counter.NextWord();
+ if (block.AllSet()) {
+ // All from left
+ CopyFixedWidth<Type>::CopyArray(*left_arr.type, left_values,
+ left_arr.offset + offset, block.length, out_values,
+ out_offset + offset);
+ } else if (block.NoneSet()) {
+ // All from right
+ CopyFixedWidth<Type>::CopyScalar(right_scalar, block.length, out_values,
+ out_offset + offset);
+ } else if (block.popcount) {
+ // One by one
+ for (int64_t j = 0; j < block.length; ++j) {
+ if (BitUtil::GetBit(left_valid, left_arr.offset + offset + j)) {
+ CopyFixedWidth<Type>::CopyArray(
+ *left_arr.type, left_values, left_arr.offset + offset + j,
+ /*length=*/1, out_values, out_offset + offset + j);
+ } else {
+ CopyFixedWidth<Type>::CopyScalar(right_scalar, /*length=*/1, out_values,
+ out_offset + offset + j);
+ }
+ }
+ }
+ offset += block.length;
+ }
+
+ if (right_scalar.is_valid || !left_valid) {
+ BitUtil::SetBitsTo(out_valid, out_offset, length, true);
+ } else {
+ arrow::internal::CopyBitmap(left_valid, left_arr.offset, length, out_valid,
+ out_offset);
+ }
+ return Status::OK();
+}
+
+// Special case: implement 'coalesce' for any 2 arguments for any fixed-width
+// type (a 'fill_null' operation)
+template <typename Type>
+Status ExecBinaryCoalesce(KernelContext* ctx, Datum left, Datum right, int64_t length,
+ Datum* out) {
+ if (left.is_scalar() && right.is_scalar()) {
+ // Both scalar
+ *out = left.scalar()->is_valid ? left : right;
+ return Status::OK();
+ }
+
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ if (left.is_scalar()) {
+ // LHS is scalar
+ CopyValues<Type>(left.scalar()->is_valid ? left : right, /*in_offset=*/0, length,
+ out_valid, out_values, out_offset);
+ return Status::OK();
+ }
+
+ // Array with nulls
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
Review comment:
Is it possible for `left_valid` to be null? If there are no nulls in the left argument, I would expect this to shortcut.
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
Review comment:
Is it possible for `left_valid` to be null?
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else_benchmark.cc
##########
@@ -386,8 +441,13 @@ BENCHMARK(CaseWhenBenchString)->Args({kFewItems, 99});
BENCHMARK(CaseWhenBenchStringContiguous)->Args({kFewItems, 0});
BENCHMARK(CaseWhenBenchStringContiguous)->Args({kFewItems, 99});
-BENCHMARK(CoalesceBench64)->Args({kNumItems, 0});
-BENCHMARK(CoalesceBench64)->Args({kNumItems, 99});
+BENCHMARK(CoalesceBench64)->Args({kNumItems, 0, 2});
+BENCHMARK(CoalesceBench64)->Args({kNumItems, 0, 4});
+BENCHMARK(CoalesceBench64)->Args({kNumItems, 99, 2});
+BENCHMARK(CoalesceBench64)->Args({kNumItems, 99, 4});
+
+BENCHMARK(CoalesceScalarBench64)->Args({kNumItems, 0});
+BENCHMARK(CoalesceScalarStringBench)->Args({kNumItems, 0});
BENCHMARK(CoalesceNonNullBench64)->Args({kNumItems, 0});
BENCHMARK(CoalesceNonNullBench64)->Args({kNumItems, 99});
Review comment:
It is possible to also parametrize the null_probability used in the coalesce benchmarks? I think we have some logic to do this (see e.g. RegressionArgs and the Take benchmarks).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r695913702
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
It was not helpful in case_when but might be useful here, I can give it a try.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm edited a comment on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
lidavidm edited a comment on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-907164879
Thanks! case_when could probably use some work, though I'm currently in the middle of digging into it to get dictionary support. select also could use some work (though the task there is harder). I'll file issues/take a look when I get a chance.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #10987:
URL: https://github.com/apache/arrow/pull/10987
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-907076442
> I think the benchmarks changed from git master, so I'm not sure there will be meaningful changes shown by conbench.
Indeed, it seems there are no significant changes in the already existing benchmarks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-907164879
Thanks! case_when could probably use some work, though I'm currently in the middle of digging into it to get dictionary support. select also could use some work (though the task there is harder).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906615407
I think the benchmarks changed from git master, so I'm not sure there will be meaningful changes shown by conbench.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-907077297
> I've submitted a PR to your branch to improve performance:
> [lidavidm#4](https://github.com/lidavidm/arrow/pull/4)
>
> I think the same approach may be used for other conditional kernels, but that would be a separate JIRA.
Apparently `if_else` is already quite optimized, so I'm not sure there's anything to be done. I didn't look at `case_when`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906612798
Benchmark runs are scheduled for baseline = 20885df121266a8519692c7630c98beb570f3b47 and contender = 43a44500991e2193a99e582d50f669ab1f6beeea. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/b0b82bffb57d4625b61eeb1d913c6373...dff104776e154921b3494fb1709d8eaf/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7e6b3303b2c4441bab00c2f4aacd238d...e6aca9e032be4c0caa030c0dfa726d69/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5d7f13b26a47454f9337081a24b98b6e...47a8ef6a5e2e4569bcde6bc342177410/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-904781611
https://issues.apache.org/jira/browse/ARROW-7179
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#issuecomment-906612798
Benchmark runs are scheduled for baseline = 20885df121266a8519692c7630c98beb570f3b47 and contender = 43a44500991e2193a99e582d50f669ab1f6beeea. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/b0b82bffb57d4625b61eeb1d913c6373...dff104776e154921b3494fb1709d8eaf/)
[Finished :arrow_down:1.46% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7e6b3303b2c4441bab00c2f4aacd238d...e6aca9e032be4c0caa030c0dfa726d69/)
[Finished :arrow_down:0.55% :arrow_up:0.51%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5d7f13b26a47454f9337081a24b98b6e...47a8ef6a5e2e4569bcde6bc342177410/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r695918177
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
It may largely depend on the clustering of nulls/non-nulls, hence the suggestion to benchmark different null probabilites.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10987: ARROW-7179: [C++][Python][R] Consolidate coalesce/fill_null
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10987:
URL: https://github.com/apache/arrow/pull/10987#discussion_r695973782
##########
File path: cpp/src/arrow/compute/kernels/scalar_if_else.cc
##########
@@ -1827,9 +1827,131 @@ Status ExecArrayCoalesce(KernelContext* ctx, const ExecBatch& batch, Datum* out)
return Status::OK();
}
+// Special case: implement 'coalesce' for an array and a scalar for any
+// fixed-width type (a 'fill_null' operation)
+template <typename Type>
+Status ExecArrayScalarCoalesce(KernelContext* ctx, Datum left, Datum right,
+ int64_t length, Datum* out) {
+ ArrayData* output = out->mutable_array();
+ const int64_t out_offset = output->offset;
+ uint8_t* out_valid = output->buffers[0]->mutable_data();
+ uint8_t* out_values = output->buffers[1]->mutable_data();
+
+ const ArrayData& left_arr = *left.array();
+ const uint8_t* left_valid =
+ left_arr.MayHaveNulls() ? left_arr.buffers[0]->data() : nullptr;
+ arrow::internal::OptionalBitBlockCounter bit_counter(left_valid, left_arr.offset,
Review comment:
In this case it does better, though I could imagine it doing badly at ~50% nulls.
```
--------------------------------------------------------------------------------------
Benchmark Time CPU Iterations UserCounters...
--------------------------------------------------------------------------------------
Before:
CoalesceScalarBench64/0 1321249 ns 1321175 ns 508 bytes_per_second=5.9133G/s items_per_second=793.67M/s length=1048.58k null%=1 num_args=2
CoalesceScalarBench64/2 5049228 ns 5049073 ns 133 bytes_per_second=1.54731G/s items_per_second=207.677M/s length=1048.58k null%=25 num_args=2
CoalesceScalarStringBench/0 87407512 ns 87404678 ns 7 bytes_per_second=5.70364G/s items_per_second=11.9968M/s length=1048.58k null%=1 num_args=2
CoalesceScalarStringBench/2 77686321 ns 77685936 ns 8 bytes_per_second=4.87622G/s items_per_second=13.4976M/s length=1048.58k null%=25 num_args=2
After:
CoalesceScalarBench64/0 988642 ns 988606 ns 664 bytes_per_second=7.90255G/s items_per_second=1060.66M/s length=1048.58k null%=1 num_args=2
CoalesceScalarBench64/2 4537487 ns 4537432 ns 154 bytes_per_second=1.72179G/s items_per_second=231.095M/s length=1048.58k null%=25 num_args=2
CoalesceScalarStringBench/0 103582667 ns 103575544 ns 6 bytes_per_second=4.81315G/s items_per_second=10.1238M/s length=1048.58k null%=1 num_args=2
CoalesceScalarStringBench/2 79813292 ns 79807705 ns 8 bytes_per_second=4.74658G/s items_per_second=13.1388M/s length=1048.58k null%=25 num_args=2
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org