You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by GitBox <gi...@apache.org> on 2022/12/13 19:30:55 UTC
[GitHub] [arrow] benibus opened a new issue, #14939: [C++] Support Table lookups in FieldRef and FieldPath
benibus opened a new issue, #14939:
URL: https://github.com/apache/arrow/issues/14939
### Describe the enhancement requested
Given that `RecordBatch` and `Array` lookups are already supported, it would useful to include `Table` as well. In practice, this would involve adding overloads for `FieldPath::Get` and `FieldRef::FindAll`
### Component(s)
C++
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace closed issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace closed issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
URL: https://github.com/apache/arrow/issues/14939
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dinimar commented on issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by "dinimar (via GitHub)" <gi...@apache.org>.
dinimar commented on issue #14939:
URL: https://github.com/apache/arrow/issues/14939#issuecomment-1460061225
@benibus I've checked src/arrow/type-test.cc file. There is no test cases related to `FieldPath::Get` or `FieldRef::FindAll` functions with RecordBatch objects.
Does these changes require new additional unit-tests for Table overloads? If it's not required then I can create PR for my changes.
--
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] VirendraYadav1234 commented on issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by GitBox <gi...@apache.org>.
VirendraYadav1234 commented on issue #14939:
URL: https://github.com/apache/arrow/issues/14939#issuecomment-1381335027
I want to work on this issues please assign me this issue
--
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] benibus commented on issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by "benibus (via GitHub)" <gi...@apache.org>.
benibus commented on issue #14939:
URL: https://github.com/apache/arrow/issues/14939#issuecomment-1464574956
Hi @dinimar. Thanks for taking this on!
In your case, each call to `get_children` is expected to return a new `const ChunkedArrayVector*` for each nesting level - whereas the current implementation just returns a single `std::shared_ptr<ChunkedArray>`. To do that, you might want to look into `ChunkedArray::Flatten`. (Keep in mind though, each new vector would need to outlive the scope of `get_children` since it's expected to return a pointer - I'd suggest using a lambda capture for that)
Handling tables likely requires some specialized logic compared to record batches due to the fact that they (uniquely) deal with `ChunkedArray`s as opposed to standard `Array`s. Of course, keeping things DRY as possible would be nice - but there will be some differences in implementation.
> Does these changes require new additional unit-tests for Table overloads?
Yes I'd say so. For `FieldPath::Get` at least.
--
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] dinimar commented on issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by "dinimar (via GitHub)" <gi...@apache.org>.
dinimar commented on issue #14939:
URL: https://github.com/apache/arrow/issues/14939#issuecomment-1464481799
@rok @pitrou @benibus Hi, I'm a newbie and stuck at implementing `FieldPath::Get(const Table & table)`
Test case code:
```
...
for (int index = 0; index < tablePtr->num_columns(); ++index) {
ASSERT_OK_AND_ASSIGN(auto fieldPathColumn, FieldPath({index}).Get(*tablePtr));
EXPECT_TRUE(fieldPathColumn->Equals(tablePtr->column(index)));
}
...
```
Currently, I have the following implementation for this function:
```
Result<std::shared_ptr<ChunkedArray>> FieldPath::Get(const Table& table) const {
ARROW_ASSIGN_OR_RAISE(auto data, FieldPathGetImpl::Get(this, table.columns()));
return data;
}
```
And the following `FieldPathGetImpl::Get` implementation:
```
static Result<std::shared_ptr<ChunkedArray>> Get(const FieldPath* path,
const ChunkedArrayVector& table) {
return FieldPathGetImpl::Get(
path, &table,
[](const std::shared_ptr<ChunkedArray>& data) {
return data;
});
}
```
For the function above I got the following compilation error:
```
\arrow\cpp\src\arrow\type.cc(1136,1): error C2440: '=': cannot convert from 'std::shared_ptr<arrow::ChunkedArray>' to 'const std::vector<std::shared_ptr<arrow::ChunkedArray>,std::allocator<std::shared_ptr<arrow::ChunkedArray>>> *'
```
related code:
```
template <typename T, typename GetChildren>
static Result<T> Get(const FieldPath* path, const std::vector<T>* children,
GetChildren&& get_children, int* out_of_range_depth) {
...
out = &children->at(index);
children = get_children(*out); // line 1136
++depth;
}
return *out;
}
```
Implementations with no using `Result<T> FieldPathGetImpl::Get(const FieldPath* path, const std::vector<T>* children,
GetChildren&& get_children, int* out_of_range_depth)` will violate DRY principle by creating non-template functions specifically for Table or putting implementation of template function right in `Get(const FieldPath* path,
const ChunkedArrayVector& table) `
Any help would be appreciated. Other functions are implemented and tested by unit tests
--
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] dinimar commented on issue #14939: [C++] Support Table lookups in FieldRef and FieldPath
Posted by "dinimar (via GitHub)" <gi...@apache.org>.
dinimar commented on issue #14939:
URL: https://github.com/apache/arrow/issues/14939#issuecomment-1464886636
@benibus Big thanks! You helped me a lot. I completed work on the issue.
PR in progress
--
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