FadyYosry77 commented on code in PR #51691:
URL: https://github.com/apache/arrow/pull/51691#discussion_r4225825411
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -1401,6 +1457,15 @@ TEST(Expression, RemoveNamedRefs) {
ExpectRemovesRefsTo(field_ref({"a", "b"}), field_ref({0, 0}), nested_schema);
}
+TEST(Expression, NullableRemoveNamedRefs) {
Review Comment:
Added in 2f2067b373: `add(required_field, literal(1))` now covers the
recursive rewrite. The field argument stays bound, positional, and
non-nullable; the call stays conservatively nullable.
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -612,6 +612,62 @@ TEST(Expression, BindNestedFieldRef) {
field("b",
int64())}))})));
}
+TEST(Expression, NullableFieldRef) {
+ auto input_schema = schema({field("r", int32(), false), field("n", int32(),
true)});
+ for (int i = 0; i < input_schema->num_fields(); ++i) {
+ for (const auto& ref : {FieldRef(input_schema->field(i)->name()),
FieldRef(i)}) {
+ auto expr = field_ref(ref);
Review Comment:
Added `SCOPED_TRACE` for the input field and reference in each direct-field
case (2f2067b373).
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -612,6 +612,62 @@ TEST(Expression, BindNestedFieldRef) {
field("b",
int64())}))})));
}
+TEST(Expression, NullableFieldRef) {
+ auto input_schema = schema({field("r", int32(), false), field("n", int32(),
true)});
+ for (int i = 0; i < input_schema->num_fields(); ++i) {
+ for (const auto& ref : {FieldRef(input_schema->field(i)->name()),
FieldRef(i)}) {
+ auto expr = field_ref(ref);
+ EXPECT_TRUE(expr.nullable());
+
+ ASSERT_OK_AND_ASSIGN(auto bound, expr.Bind(*input_schema));
+ EXPECT_EQ(bound.nullable(), input_schema->field(i)->nullable());
+ EXPECT_TRUE(expr.nullable());
+
+ ASSERT_OK_AND_ASSIGN(auto bound_to_type,
+ expr.Bind(struct_(input_schema->fields())));
+ EXPECT_EQ(bound_to_type.nullable(), bound.nullable());
+ }
+ }
+}
+
+TEST(Expression, NullableConservativeFallback) {
+ auto input_schema = schema({field("r", int32(), false), field("n", int32(),
true)});
+ EXPECT_TRUE(Expression{}.nullable());
+ for (const auto& expr : {literal(1),
literal(std::make_shared<Int32Scalar>()),
+ add(field_ref("r"), literal(1)),
is_valid(field_ref("n"))}) {
+ EXPECT_TRUE(expr.nullable());
Review Comment:
Added `SCOPED_TRACE(expr.ToString())` for each fallback expression
(2f2067b373).
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -612,6 +612,62 @@ TEST(Expression, BindNestedFieldRef) {
field("b",
int64())}))})));
}
+TEST(Expression, NullableFieldRef) {
+ auto input_schema = schema({field("r", int32(), false), field("n", int32(),
true)});
+ for (int i = 0; i < input_schema->num_fields(); ++i) {
+ for (const auto& ref : {FieldRef(input_schema->field(i)->name()),
FieldRef(i)}) {
+ auto expr = field_ref(ref);
+ EXPECT_TRUE(expr.nullable());
+
+ ASSERT_OK_AND_ASSIGN(auto bound, expr.Bind(*input_schema));
+ EXPECT_EQ(bound.nullable(), input_schema->field(i)->nullable());
+ EXPECT_TRUE(expr.nullable());
+
+ ASSERT_OK_AND_ASSIGN(auto bound_to_type,
+ expr.Bind(struct_(input_schema->fields())));
+ EXPECT_EQ(bound_to_type.nullable(), bound.nullable());
+ }
+ }
+}
+
+TEST(Expression, NullableConservativeFallback) {
+ auto input_schema = schema({field("r", int32(), false), field("n", int32(),
true)});
+ EXPECT_TRUE(Expression{}.nullable());
+ for (const auto& expr : {literal(1),
literal(std::make_shared<Int32Scalar>()),
+ add(field_ref("r"), literal(1)),
is_valid(field_ref("n"))}) {
+ EXPECT_TRUE(expr.nullable());
+ ASSERT_OK_AND_ASSIGN(auto bound, expr.Bind(*input_schema));
+ EXPECT_TRUE(bound.IsBound());
+ EXPECT_TRUE(bound.nullable());
+ }
+}
+
+TEST(Expression, NullableNestedFieldRef) {
+ for (bool parent_nullable : {false, true}) {
+ for (bool child_nullable : {false, true}) {
+ auto input_schema = schema(
+ {field("a", struct_({field("b", int32(), child_nullable)}),
parent_nullable)});
+ for (const auto& ref : {FieldRef("a", "b"), FieldRef(FieldPath({0,
0}))}) {
+ ASSERT_OK_AND_ASSIGN(auto bound, field_ref(ref).Bind(*input_schema));
Review Comment:
Added `SCOPED_TRACE` for the schema and reference in each nested-field case
(2f2067b373).
--
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]
