Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43768: [C++] Fix the case when boolean_{any|all} meets constant input with length in Acero #43799

Merged
merged 5 commits into from
Sep 2, 2024
Merged

GH-43768: [C++] Fix the case when boolean_{any|all} meets constant input with length in Acero #43799

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6120da4
Fix the case when boolean_{any|all} meets constant input with length
mapleFU Aug 23, 2024
74191a4
Update cpp/src/arrow/acero/aggregate_node_test.cc
mapleFU Aug 23, 2024
e700177
Also fixing the BooleanAll array handling
mapleFU Aug 23, 2024
cab4b25
Merge branch 'main' into boolean-agg-fix
mapleFU Aug 29, 2024
140e3be
more tests
mapleFU Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions cpp/src/arrow/acero/aggregate_node_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,5 +210,36 @@ TEST(GroupByNode, NoSkipNulls) {
AssertExecBatchesEqualIgnoringOrder(out_schema, {expected_batch}, out_batches.batches);
}

TEST(ScalarAggregateNode, AnyAll) {
// GH-43768: boolean_any and boolean_all with constant input should work well
// when min_count != 0.
std::shared_ptr<Schema> in_schema = schema({field("not_used", int32())});
std::shared_ptr<Schema> out_schema = schema({field("agg_out", boolean())});
std::vector<ExecBatch> batches{
ExecBatchFromJSON({int32()}, "[[42], [42], [42], [42]]")};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test more cases? Something like:

  struct AnyAllCase {
    std::string batches_json;
    std::string expected_json;
  };
  std::vector<AnyAllCase> cases {
    {"[[42], []]", "[[false]]"},
    {"[[42, 42], []]", "[[true]]"},
    {"[[42], [42]]", "[[true]]"},
    {"[[42], [42, 42]]", "[[true]]"},
    {"[[42], []]", "[[false]]"},
    {"[[42, 42], []]", "[[true]]"},
    {"[[42], [42]]", "[[true]]"},
    {"[[42], [42, 42]]", "[[true]]"},
  };

Also vary the skip_nulls and the boolean literal.

Copy link
Member Author

@mapleFU mapleFU Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit hard to build test like this since the Literal column would only built by Projection here, but I've tried

Project cannot make "different" literal in this scenerio

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou Would you mind take a quick look that whether the testing is ok?

for (auto& func_name : {"any", "all"}) {
SCOPED_TRACE(func_name);
std::vector<Aggregate> aggregates = {
Aggregate(func_name,
std::make_shared<compute::ScalarAggregateOptions>(/*skip_nulls=*/false,
/*min_count=*/2),
FieldRef("literal_true"))};

// And a projection to make the input including a Scalar Boolean
Declaration plan = Declaration::Sequence(
{{"exec_batch_source", ExecBatchSourceNodeOptions(in_schema, batches)},
{"project", ProjectNodeOptions({literal(true)}, {"literal_true"})},
{"aggregate", AggregateNodeOptions(aggregates)}});

ASSERT_OK_AND_ASSIGN(BatchesWithCommonSchema out_batches,
DeclarationToExecBatches(plan));

ExecBatch expected_batch = ExecBatchFromJSON({boolean()}, "[[true]]");

AssertExecBatchesEqualIgnoringOrder(out_schema, {expected_batch},
out_batches.batches);
}
}

} // namespace acero
} // namespace arrow
16 changes: 8 additions & 8 deletions cpp/src/arrow/compute/kernels/aggregate_basic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,13 +532,13 @@ struct BooleanAnyImpl : public ScalarAggregator {
}
if (batch[0].is_scalar()) {
const Scalar& scalar = *batch[0].scalar;
this->has_nulls = !scalar.is_valid;
this->any = scalar.is_valid && checked_cast<const BooleanScalar&>(scalar).value;
this->count += scalar.is_valid;
this->has_nulls |= !scalar.is_valid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? If the argument is a scalar, isn't it as if the whole batch was made from repeated values of that scalar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If previous is has_null, it always has_null ( that's why using |= )
  2. !scalar.is_valid means has-null, so it would set by this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @mapleFU is right here. this->has_nulls is meant to represent whether any null was encountered, so it should never change from true to false.

this->any |= scalar.is_valid && checked_cast<const BooleanScalar&>(scalar).value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the |? Isn't that necessary only in MergeFrom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #43799 (comment)

this->count += scalar.is_valid * batch.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I understand.

return Status::OK();
}
const ArraySpan& data = batch[0].array;
this->has_nulls = data.GetNullCount() > 0;
this->has_nulls |= data.GetNullCount() > 0;
this->count += data.length - data.GetNullCount();
arrow::internal::OptionalBinaryBitBlockCounter counter(
data.buffers[0].data, data.offset, data.buffers[1].data, data.offset,
Expand Down Expand Up @@ -603,13 +603,13 @@ struct BooleanAllImpl : public ScalarAggregator {
}
if (batch[0].is_scalar()) {
const Scalar& scalar = *batch[0].scalar;
this->has_nulls = !scalar.is_valid;
this->count += scalar.is_valid;
this->all = !scalar.is_valid || checked_cast<const BooleanScalar&>(scalar).value;
this->has_nulls |= !scalar.is_valid;
this->count += scalar.is_valid * batch.length;
this->all &= !scalar.is_valid || checked_cast<const BooleanScalar&>(scalar).value;
return Status::OK();
}
const ArraySpan& data = batch[0].array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't change has_null = to has_null |= as what you did for any?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And my question is why make it |= instead of simply =?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is in #43799 (comment)

this->has_nulls = data.GetNullCount() > 0;
this->has_nulls |= data.GetNullCount() > 0;
this->count += data.length - data.GetNullCount();
arrow::internal::OptionalBinaryBitBlockCounter counter(
data.buffers[1].data, data.offset, data.buffers[0].data, data.offset,
Expand Down
Loading