You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/10 14:07:53 UTC
[GitHub] [arrow] pitrou commented on a diff in pull request #14338: ARROW-17956: [C++] RandomArrayGenerator does not properly generate ListArrays with Nulls
pitrou commented on code in PR #14338:
URL: https://github.com/apache/arrow/pull/14338#discussion_r991320041
##########
cpp/src/arrow/testing/random_test.cc:
##########
@@ -343,6 +343,47 @@ TEST(TypeSpecificTests, ListLengths) {
}
}
+// ARROW-17956: Two cases to cases to make sure the RandomArrayGenerator properly
+// generates lists with nulls:
+// - a ListArray where every member is null (null_probability 1.0). Previously the
+// OffsetsFromLengthsArray method used internally did not handle arrays where the first
+// or last element is Null, which defniedly are null for this array
Review Comment:
"definitely" perhaps?
##########
cpp/src/arrow/testing/random_test.cc:
##########
@@ -343,6 +343,47 @@ TEST(TypeSpecificTests, ListLengths) {
}
}
+// ARROW-17956: Two cases to cases to make sure the RandomArrayGenerator properly
+// generates lists with nulls:
+// - a ListArray where every member is null (null_probability 1.0). Previously the
+// OffsetsFromLengthsArray method used internally did not handle arrays where the first
+// or last element is Null, which defniedly are null for this array
+// - a ListArray where members are either null or have a known length (10). This is to
+// test there is
Review Comment:
Fix indentation/word-wrapping here?
##########
cpp/src/arrow/testing/random.cc:
##########
@@ -721,13 +709,11 @@ std::shared_ptr<Array> RandomArrayGenerator::ArrayOf(const Field& field, int64_t
for (const auto& length : *lengths) { \
if (length.has_value()) values_length += *length; \
} \
- const auto force_empty_nulls = \
- GetMetadata<bool>(field.metadata().get(), "force_empty_nulls", false); \
const auto values = \
ArrayOf(*internal::checked_pointer_cast<ARRAY_TYPE::TypeClass>(field.type()) \
->value_field(), \
values_length); \
- const auto offsets = OffsetsFromLengthsArray(lengths.get(), force_empty_nulls); \
+ const auto offsets = OffsetsFromLengthsArray(lengths.get()); \
Review Comment:
Hmm... instead of fixing `OffsetsFromLengthsArray` and ignoring `force_empty_nulls`, why not use `GenerateOffsets` or even `RandomArrayGenerator::List`?
##########
cpp/src/arrow/testing/random_test.cc:
##########
@@ -343,6 +343,47 @@ TEST(TypeSpecificTests, ListLengths) {
}
}
+// ARROW-17956: Two cases to cases to make sure the RandomArrayGenerator properly
Review Comment:
"Two cases to cases"?
--
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