[GitHub] [arrow] westonpace commented on a diff in pull request #34537: GH-14939: [C++] Support Table lookups in FieldRef and FieldPath

["westonpace (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org