You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "westonpace (via GitHub)" <gi...@apache.org> on 2023/03/22 18:55:44 UTC
[GitHub] [arrow] westonpace opened a new pull request, #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
westonpace opened a new pull request, #34691:
URL: https://github.com/apache/arrow/pull/34691
### Rationale for this change
A struct array can have a validity map and an offset. A record batch cannot. When converting from a struct array to a record batch we were throwing an error if a validity map was present and returning the wrong data if an offset was present.
### What changes are included in this PR?
If a validity map or offset are present then StructArray::Flatten is used to push the offset and validity map into the struct array. Note: this means that RecordBatch::FromStructArray will not be zero-copy if it has to push down a validity map.
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes, RecordBatch::FromStructArray now takes in a memory pool because it might have to make allocations when pushing validity bitmaps down.
--
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] jorisvandenbossche commented on pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #34691:
URL: https://github.com/apache/arrow/pull/34691#issuecomment-1481305606
Sounds good to me.
I don't know if there is a use case for wanting it to be zero-copy (like in the conversion to numpy there is a zero_copy_only keyword that avoids doing the conversion if it would trigger a copy). But in the end, the user can quite easily check the null_count of the parent struct array if they for some reason would want to raise an error in that 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] westonpace commented on a diff in pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1147757734
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -198,14 +198,20 @@ Result<std::shared_ptr<RecordBatch>> RecordBatch::MakeEmpty(
}
Result<std::shared_ptr<RecordBatch>> RecordBatch::FromStructArray(
- const std::shared_ptr<Array>& array) {
+ const std::shared_ptr<Array>& array, MemoryPool* memory_pool) {
if (array->type_id() != Type::STRUCT) {
return Status::TypeError("Cannot construct record batch from array of type ",
*array->type());
}
- if (array->null_count() != 0) {
- return Status::Invalid(
- "Unable to construct record batch from a StructArray with non-zero nulls.");
+ if (array->null_count() != 0 || array->offset() != 0) {
+ // If the struct array has a validity map or offset we need to push those into
Review Comment:
Good question but I think we are ok. If there is only an offset then I believe `StructArray::Flatten` is zero-copy. I think the only case that allocates is when [both arrays have a non-trivial validity bitmap](https://github.com/apache/arrow/blob/apache-arrow-11.0.0/cpp/src/arrow/array/array_nested.cc#L636-L641).
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1149639189
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -198,14 +198,20 @@ Result<std::shared_ptr<RecordBatch>> RecordBatch::MakeEmpty(
}
Result<std::shared_ptr<RecordBatch>> RecordBatch::FromStructArray(
- const std::shared_ptr<Array>& array) {
+ const std::shared_ptr<Array>& array, MemoryPool* memory_pool) {
Review Comment:
Good catch. Fortunately it seems we don't use this method too much. I forwarded the parameter to two call sites in the JSON reader (although I think we will always be zero-copy in those cases it doesn't hurt). There is one more call site in Datum::MakeExecBatch but it was already missing a memory pool (and implicitly using the default in other calls, e.g. cast) so I just left it as is. I'm also not sure this code path is used anywhere.
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34691:
URL: https://github.com/apache/arrow/pull/34691
--
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] felipecrv commented on a diff in pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1147797323
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -198,14 +198,20 @@ Result<std::shared_ptr<RecordBatch>> RecordBatch::MakeEmpty(
}
Result<std::shared_ptr<RecordBatch>> RecordBatch::FromStructArray(
- const std::shared_ptr<Array>& array) {
+ const std::shared_ptr<Array>& array, MemoryPool* memory_pool) {
Review Comment:
It might be a good idea to review the callsites to this function and ensure they pass the context's memory pool instead of just passing the default one automatically.
--
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 pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34691:
URL: https://github.com/apache/arrow/pull/34691#issuecomment-1480101627
CC @felipecrv for review
--
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] jorisvandenbossche commented on a diff in pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1146291062
##########
cpp/src/arrow/record_batch.h:
##########
@@ -82,10 +82,15 @@ class ARROW_EXPORT RecordBatch {
/// \brief Construct record batch from struct array
///
/// This constructs a record batch using the child arrays of the given
- /// array, which must be a struct array. Note that the struct array's own
- /// null bitmap is not reflected in the resulting record batch.
+ /// array, which must be a struct array.
+ ///
+ /// \param[in] pool the memory pool to allocate new validity bitmaps
Review Comment:
Because you added the `pool` param documentation, it now seems to complain that `array` is not documented
--
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] wgtmac commented on a diff in pull request #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1145581071
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -198,14 +198,20 @@ Result<std::shared_ptr<RecordBatch>> RecordBatch::MakeEmpty(
}
Result<std::shared_ptr<RecordBatch>> RecordBatch::FromStructArray(
- const std::shared_ptr<Array>& array) {
+ const std::shared_ptr<Array>& array, MemoryPool* memory_pool) {
if (array->type_id() != Type::STRUCT) {
return Status::TypeError("Cannot construct record batch from array of type ",
*array->type());
}
- if (array->null_count() != 0) {
- return Status::Invalid(
- "Unable to construct record batch from a StructArray with non-zero nulls.");
+ if (array->null_count() != 0 || array->offset() != 0) {
+ // If the struct array has a validity map or offset we need to push those into
Review Comment:
If the struct_array only has an offset (w/o validity map), does `struct_array->Slice(offset, length)` work as well which should be zero copy?
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34691:
URL: https://github.com/apache/arrow/pull/34691#issuecomment-1480101610
:warning: GitHub issue #34639 **has been automatically assigned in GitHub** to PR creator.
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34691:
URL: https://github.com/apache/arrow/pull/34691#issuecomment-1480101565
* Closes: #34639
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34691:
URL: https://github.com/apache/arrow/pull/34691#discussion_r1147760534
##########
cpp/src/arrow/record_batch.h:
##########
@@ -82,10 +82,15 @@ class ARROW_EXPORT RecordBatch {
/// \brief Construct record batch from struct array
///
/// This constructs a record batch using the child arrays of the given
- /// array, which must be a struct array. Note that the struct array's own
- /// null bitmap is not reflected in the resulting record batch.
+ /// array, which must be a struct array.
+ ///
+ /// \param[in] pool the memory pool to allocate new validity bitmaps
Review Comment:
I've now documented this.
--
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 #34691: GH-34639: [C++] Support RecordBatch::FromStructArray even if struct array has nulls/offsets
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34691:
URL: https://github.com/apache/arrow/pull/34691#issuecomment-1486011062
Benchmark runs are scheduled for baseline = 8d1863a76ea08511db546facec9d86a1a23633c0 and contender = 196fbe5f5e0669b1f8bf71005c8a4c6a4e0fb2da. 196fbe5f5e0669b1f8bf71005c8a4c6a4e0fb2da 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/22319965f89247d2a45114d28a71ad27...28961df07e034a3da70ad1904a218184/)
[Failed :arrow_down:0.06% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8a2746cfa5d64ab58c4168cb4092dea0...71883c62682e4230a119b36133c50da0/)
[Finished :arrow_down:0.26% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ff1d9cefdb6d421bad6ca2285ac02a58...f1abaff0cf41447f8e1e356b8739b8e0/)
[Finished :arrow_down:1.11% :arrow_up:0.43%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2edea3e2d7b74d66a26266c36db7ee43...c38bd549ed1342e197238b85123ba1ee/)
Buildkite builds:
[Finished] [`196fbe5f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2584)
[Failed] [`196fbe5f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2614)
[Finished] [`196fbe5f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2582)
[Finished] [`196fbe5f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2605)
[Finished] [`8d1863a7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2583)
[Failed] [`8d1863a7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2613)
[Finished] [`8d1863a7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2581)
[Finished] [`8d1863a7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2604)
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