benibus commented on code in PR #34537:
URL: https://github.com/apache/arrow/pull/34537#discussion_r1142701192
##########
cpp/src/arrow/type.cc:
##########
@@ -1204,6 +1221,12 @@ Result<std::shared_ptr<Array>> FieldPath::Get(const
RecordBatch& batch) const {
return MakeArray(std::move(data));
}
+Result<std::shared_ptr<ChunkedArray>> FieldPath::Get(const Table& table) const
{
+ ARROW_ASSIGN_OR_RAISE(auto data, FieldPathGetImpl::Get(this,
table.columns()));
+ ARROW_CHECK_OK(data->ValidateFull());
Review Comment:
`ValidateFull` is very expensive so I'd avoid using outside of the tests
(generally speaking). In this case, it probably isn't necessary.
##########
cpp/src/arrow/type.cc:
##########
@@ -1159,6 +1161,21 @@ struct FieldPathGetImpl {
});
}
+ static Result<std::shared_ptr<ChunkedArray>> Get(const FieldPath* path,
+ const ChunkedArrayVector&
table) {
+ arrow::ChunkedArrayVector chunked_array_vector_buffer;
+
+ return FieldPathGetImpl::Get(
+ path, &table,
+ [&chunked_array_vector_buffer]
+ (const std::shared_ptr<ChunkedArray>& data) -> const ChunkedArrayVector
* {
+ auto chunked_array_vector = data->Flatten();
+
+ chunked_array_vector_buffer = chunked_array_vector.ValueUnsafe();
+ return &chunked_array_vector_buffer;
+ });
Review Comment:
Something like this would be more useful.
```suggestion
Status status;
auto result = FieldPathGetImpl::Get(
path, &table,
[&chunked_array_vector_buffer, &status]
(const std::shared_ptr<ChunkedArray>& data) -> const
ChunkedArrayVector * {
auto maybe_chunked_array_vector = data->Flatten();
if (!maybe_chunked_array_vector.ok()) {
status = maybe_chunked_array_vector.status();
return nullptr;
}
chunked_array_vector_buffer =
maybe_chunked_array_vector.MoveValueUnsafe();
return &chunked_array_vector_buffer;
});
ARROW_RETURN_NOT_OK(status);
return result;
```
Alternatively, you could add error handling to `FieldPathGetImpl::Get`
[here](https://github.com/apache/arrow/blob/62866580803ff9537ddaf871a76a68b9c8837b4e/cpp/src/arrow/type.cc#L1137),
as in
```
ARROW_ASSIGN_OR_RAISE(children, get_children(*out));
```
and then modify the `get_children` implementations to return a `Result`.
##########
cpp/src/arrow/type.cc:
##########
@@ -1159,6 +1161,21 @@ struct FieldPathGetImpl {
});
}
+ static Result<std::shared_ptr<ChunkedArray>> Get(const FieldPath* path,
+ const ChunkedArrayVector&
table) {
+ arrow::ChunkedArrayVector chunked_array_vector_buffer;
+
+ return FieldPathGetImpl::Get(
+ path, &table,
+ [&chunked_array_vector_buffer]
+ (const std::shared_ptr<ChunkedArray>& data) -> const ChunkedArrayVector
* {
Review Comment:
Somewhat nitpicky, but can we use different terminology here rather than
`data`? The other functions use it because they work with `ArrayData`
specifically, but here we're dealing with `ChunkedArray`s - which are a higher
level concept.
--
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]
