jorisvandenbossche commented on code in PR #14697:
URL: https://github.com/apache/arrow/pull/14697#discussion_r1038168326
##########
cpp/src/arrow/type_test.cc:
##########
@@ -434,6 +434,51 @@ TEST(TestFieldRef, DotPathRoundTrip) {
check_roundtrip(FieldRef("foo", 1, FieldRef("bar", 2, 3), FieldRef()));
}
+TEST(TestFieldRef, NestedWithList) {
+ // bit of indirection to satisfy list / large_list variants
+ auto gen_type = [](std::shared_ptr<DataType> (*f)(const
std::shared_ptr<DataType>&)) {
+ return f;
+ };
+
+ for (auto list_type : {gen_type(list), gen_type(large_list)}) {
+ // Single list type
+ auto type = struct_(
+ {field("c", list_type(struct_({field("d", int32()), field("e",
int8())})))});
+
+ // Note numeric values here go unused, outside of indicating a further
step, during
+ // FindAll, only used by kernels for which list element to take. When
FindAll
+ // encounters a list, it assumes the list value type. Not b/c they are
numeric, but
+ // they are the position referencing a specific list element.
+ EXPECT_THAT(FieldRef("c", 0, "d").FindAll(*type), ElementsAre(FieldPath{0,
0, 0}));
+ EXPECT_THAT(FieldRef("c", 0, "e").FindAll(*type), ElementsAre(FieldPath{0,
0, 1}));
+ EXPECT_THAT(FieldRef("c", 1, "d").FindAll(*type), ElementsAre(FieldPath{0,
1, 0}));
+ EXPECT_THAT(FieldRef("c", 1, "e").FindAll(*type), ElementsAre(FieldPath{0,
1, 1}));
Review Comment:
For completeness, can you also add a variant where the type starts with a
list?
##########
cpp/src/arrow/type_test.cc:
##########
@@ -434,6 +434,51 @@ TEST(TestFieldRef, DotPathRoundTrip) {
check_roundtrip(FieldRef("foo", 1, FieldRef("bar", 2, 3), FieldRef()));
}
+TEST(TestFieldRef, NestedWithList) {
+ // bit of indirection to satisfy list / large_list variants
+ auto gen_type = [](std::shared_ptr<DataType> (*f)(const
std::shared_ptr<DataType>&)) {
+ return f;
+ };
+
+ for (auto list_type : {gen_type(list), gen_type(large_list)}) {
+ // Single list type
+ auto type = struct_(
+ {field("c", list_type(struct_({field("d", int32()), field("e",
int8())})))});
+
+ // Note numeric values here go unused, outside of indicating a further
step, during
+ // FindAll, only used by kernels for which list element to take. When
FindAll
+ // encounters a list, it assumes the list value type. Not b/c they are
numeric, but
+ // they are the position referencing a specific list element.
+ EXPECT_THAT(FieldRef("c", 0, "d").FindAll(*type), ElementsAre(FieldPath{0,
0, 0}));
+ EXPECT_THAT(FieldRef("c", 0, "e").FindAll(*type), ElementsAre(FieldPath{0,
0, 1}));
+ EXPECT_THAT(FieldRef("c", 1, "d").FindAll(*type), ElementsAre(FieldPath{0,
1, 0}));
+ EXPECT_THAT(FieldRef("c", 1, "e").FindAll(*type), ElementsAre(FieldPath{0,
1, 1}));
Review Comment:
Maybe also cover "failure" cases like `FieldRef("c", "non-integer",
"d").FindAll(*type)` where there is not an integer index at the location of
the list level.
--
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]
