westonpace commented on code in PR #34537:
URL: https://github.com/apache/arrow/pull/34537#discussion_r1149898831
##########
cpp/src/arrow/type.cc:
##########
@@ -1216,6 +1321,18 @@ Result<std::shared_ptr<ArrayData>> FieldPath::Get(const
ArrayData& data) const {
return FieldPathGetImpl::Get(this, data.child_data);
}
+Result<std::shared_ptr<ChunkedArray>> FieldPath::Get(
+ const ChunkedArray& chunked_array) const {
+ if (chunked_array.num_chunks() < 1) {
+ return Status::Invalid("Chunked array must have at least one chunk");
Review Comment:
Why?
##########
cpp/src/arrow/type_test.cc:
##########
@@ -379,6 +382,168 @@ TEST(TestFieldPath, Basics) {
FieldPath({s.num_fields() * 2}).Get(s));
}
+TEST(TestFieldPath, GetForTable) {
+ using testing::HasSubstr;
+
+ const int length = 4;
Review Comment:
```suggestion
constexpr int kLength = 4;
```
Very minor, but `kLength` will make it clear elsewhere it is a constant (I
don't really care too much about the `const` -> `constexpr` thing but probably
a good habit to get into :)
I'd also consider `kNumRows` instead of `kLength` as it is more obvious to
the reader.
Note, this same comment applies in a few places below too.
##########
cpp/src/arrow/type_test.cc:
##########
@@ -398,6 +563,71 @@ TEST(TestFieldRef, Basics) {
EXPECT_THAT(FieldRef("beta").FindAll(s), ElementsAre(FieldPath{1},
FieldPath{3}));
}
+TEST(TestFieldRef, FindAllForTable) {
Review Comment:
Let's create a github issue for the generalization of these tests (I agree
that is a good idea and easier to maintain). If we want to leave these tests
in for now then we should mention (in the issue) that these tests can be
replaced when the generic approach is implemented.
--
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]
