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 2022/11/16 13:53:17 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
rtpsw opened a new pull request, #14658:
URL: https://github.com/apache/arrow/pull/14658
See https://issues.apache.org/jira/browse/ARROW-18342
--
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 #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14658:
URL: https://github.com/apache/arrow/pull/14658#issuecomment-1317059248
https://issues.apache.org/jira/browse/ARROW-18342
--
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] westonpace commented on a diff in pull request #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14658:
URL: https://github.com/apache/arrow/pull/14658#discussion_r1025738098
##########
cpp/src/arrow/compute/exec/asof_join_node_test.cc:
##########
@@ -91,6 +93,18 @@ Result<BatchesWithSchema> MakeBatchesFromNumString(
ARROW_ASSIGN_OR_RAISE(Datum as_string, Cast(num_batch.values[i], utf8()));
ARROW_ASSIGN_OR_RAISE(Datum as_type, Cast(as_string, type));
values.push_back(as_type);
+ } else if (Type::BOOL == type->id()) {
+ // the next 4 lines compute `as_bool` as `(bool)(x - 2*(x/2))`, i.e., the low bit
+ // of `x`. Here, `x` stands for `num_batch.values[i]`, which is an `int64` value.
+ // Taking the low bit is a somewhat arbitrary way of obtaining both `true` and
+ // `false` values from the `int64` values in the test data, in order to get good
+ // testing coverage. A simple cast to a Boolean value would not get good coverage
+ // because all positive values would be cast to `true`.
+ ARROW_ASSIGN_OR_RAISE(Datum div_two, Divide(num_batch.values[i], two));
+ ARROW_ASSIGN_OR_RAISE(Datum rounded, Multiply(div_two, two));
+ ARROW_ASSIGN_OR_RAISE(Datum low_bit, Subtract(num_batch.values[i], rounded));
+ ARROW_ASSIGN_OR_RAISE(Datum as_bool, Cast(low_bit, type));
Review Comment:
Hmm, we have a bit_wise_and kernel but I don't think there is a C++ helper function (e.g. `BitWiseAnd`) wrapping it. This is probably fine.
--
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] westonpace merged pull request #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #14658:
URL: https://github.com/apache/arrow/pull/14658
--
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 #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14658:
URL: https://github.com/apache/arrow/pull/14658#issuecomment-1319549247
Benchmark runs are scheduled for baseline = 6697826746c1c871bb673a94299d3a235f20ff2e and contender = 84c9ac73be6b157dbc80ed93ef6dd2a74ee861af. 84c9ac73be6b157dbc80ed93ef6dd2a74ee861af is a master commit associated with this PR. 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/954a418ff1474c4aa0bb35658ece3dda...5188b3d7fa1642fda810665369e04f0f/)
[Failed :arrow_down:0.37% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8181daad17b24540b5cea9a80a43bd98...bb1d1a18402a49aba1393d7c102f970c/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/82ecd8a49cd6489ab05813897f90d952...1a0b858642d2415ebbdc86ce290b4a55/)
[Finished :arrow_down:0.35% :arrow_up:0.14%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a14a3394363249d1908979f455d921a9...a2ab498d90924345bf8f31d263fccf17/)
Buildkite builds:
[Finished] [`84c9ac73` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1891)
[Failed] [`84c9ac73` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1913)
[Failed] [`84c9ac73` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1882)
[Finished] [`84c9ac73` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1905)
[Finished] [`66978267` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1890)
[Failed] [`66978267` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1912)
[Failed] [`66978267` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1881)
[Finished] [`66978267` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1904)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] wjones127 commented on a diff in pull request #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #14658:
URL: https://github.com/apache/arrow/pull/14658#discussion_r1024345220
##########
cpp/src/arrow/compute/exec/asof_join_node_test.cc:
##########
@@ -83,6 +85,7 @@ Result<BatchesWithSchema> MakeBatchesFromNumString(
batches.schema = schema;
int n_fields = schema->num_fields();
for (auto num_batch : num_batches.batches) {
+ Datum two(ConstantArrayGenerator::Int32(num_batch.length, 2));
Review Comment:
Why not a scalar here? (and remove `#include "arrow/testing/generator.h"` above)
```suggestion
Datum two(Int32Scalar(2));
```
##########
cpp/src/arrow/compute/exec/asof_join_node.cc:
##########
@@ -664,12 +666,25 @@ class CompositeReferenceTable {
}
template <class Type, class Builder = typename TypeTraits<Type>::BuilderType>
- enable_if_fixed_width_type<Type, Status> static BuilderAppend(
+ enable_if_boolean<Type, Status> static BuilderAppend(
Builder& builder, const std::shared_ptr<ArrayData>& source, row_index_t row) {
if (source->IsNull(row)) {
builder.UnsafeAppendNull();
return Status::OK();
}
+ builder.UnsafeAppend(bit_util::GetBit(source->template GetValues<uint8_t>(1), row));
+ return Status::OK();
+ }
+
+ template <class Type, class Builder = typename TypeTraits<Type>::BuilderType>
+ enable_if_t<is_fixed_width_type<Type>::value && !is_boolean_type<Type>::value,
+ Status> static BuilderAppend(Builder& builder,
+ const std::shared_ptr<ArrayData>& source,
+ row_index_t row) {
+ if (source->IsNull(row)) {
+ builder.UnsafeAppendNull();
+ return Status::OK();
+ }
Review Comment:
One pattern we've been moving to in the code base is using constexpr conditions:
```suggestion
template <class Type, class Builder = typename TypeTraits<Type>::BuilderType>
enable_if_fixed_width_type<Type, Status> static BuilderAppend(Builder& builder,
const std::shared_ptr<ArrayData>& source,
row_index_t row) {
if (source->IsNull(row)) {
builder.UnsafeAppendNull();
return Status::OK();
}
if constexpr (is_boolean_type<Type>::value) {
builder.UnsafeAppend(bit_util::GetBit(source->template GetValues<uint8_t>(1), row));
} else {
using CType = typename TypeTraits<Type>::CType;
builder.UnsafeAppend(source->template GetValues<CType>(1)[row]);
}
```
(Note this suggestion is incomplete, since GitHub limits which lines I can diff.)
--
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