zanmato1984 commented on code in PR #51691:
URL: https://github.com/apache/arrow/pull/51691#discussion_r4213490664
##########
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:
Nit (non-blocking): Please also cover a required field inside a call, for
example add(required_field, literal(1)). After RemoveNamedRefs, check that the
transformed field argument is still bound, uses a positional reference, and
retains nullable=false. This would protect nullability preservation through the
recursive rewrite path, in addition to the direct-field case here. The call
itself should remain 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);
+ 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:
Nit (non-blocking): Identify the expression when one of these fallback cases
fails.
```suggestion
SCOPED_TRACE(expr.ToString());
EXPECT_TRUE(expr.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:
Nit (non-blocking): Add the input field and reference to failure diagnostics
for each case.
```suggestion
SCOPED_TRACE(input_schema->field(i)->ToString());
SCOPED_TRACE(ref.ToString());
auto expr = field_ref(ref);
```
##########
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:
Nit (non-blocking): Include the schema and reference so failures identify
both parent/child nullability and the reference form.
```suggestion
SCOPED_TRACE(input_schema->ToString());
SCOPED_TRACE(ref.ToString());
ASSERT_OK_AND_ASSIGN(auto bound, field_ref(ref).Bind(*input_schema));
```
--
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]
