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

[mapleFU]

Rationale for this change

See #43768

What changes are included in this PR?

Fix the case when boolean_{any|all} meets constant input with length in Acero

Are these changes tested?

Yes

Are there any user-facing changes?

no

• GitHub Issue: [C++] Compute: Why some ScalarAggregator handles scalar ExecSpan like single value, other like groups value? #43768

[github-actions]

⚠️ GitHub issue #43768 has been automatically assigned in GitHub to PR creator.

[mapleFU]

@zanmato1984 @pitrou @felipecrv would you mind take a look?

[zanmato1984]

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

[mapleFU]

done

[felipecrv]

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

[mapleFU]

The reason is in #43799 (comment)

[zanmato1984]

LGTM

[felipecrv]

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?

[mapleFU]

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

[pitrou]

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.

[felipecrv]

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

[mapleFU]

See #43799 (comment)

[felipecrv]

This one I understand.

[felipecrv]

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

[mapleFU]

arrow/cpp/src/arrow/acero/scalar_aggregate_node.cc

Line 222 in e61c105

RETURN_NOT_OK(kernels_[i]->consume(&batch_ctx, column_batch));

@felipecrv The problem is here, the ScalarAggregateExecutor would do this well, but acero would continue using consume rather than handle each batch separately

[felipecrv]

arrow/cpp/src/arrow/acero/scalar_aggregate_node.cc

Line 222 in e61c105

RETURN_NOT_OK(kernels_[i]->consume(&batch_ctx, column_batch));

@felipecrv The problem is here, the ScalarAggregateExecutor would do this well, but acero would continue using consume rather than handle each batch separately

Isn't that a problem in itself?

[mapleFU]

Isn't that a problem in itself?

IMO we don't declare that SCALAR_AGGREGATE is for "each call consume once, and merge into a final state". So we'd better not expect that?

/// \brief Kernel data structure for implementations of
/// ScalarAggregateFunction. The four necessary components of an aggregation
/// kernel are the init, consume, merge, and finalize functions.
///
/// * init: creates a new KernelState for a kernel.
/// * consume: processes an ExecSpan and updates the KernelState found in the
///   KernelContext.
/// * merge: combines one KernelState with another.
/// * finalize: produces the end result of the aggregation using the
///   KernelState in the KernelContext.
struct ARROW_EXPORT ScalarAggregateKernel : public Kernel {

The ScalarAggregateKernel says "merge" and "consume" does their own thing, but not saying that "every consume would be call once and merge to a function", which is the logic in ScalarAggregateExecutor

I would also glad to put the acero with other method but I think modify Boolean{Any|All} would be ok and no more cost?

@felipecrv

[felipecrv]

Makes sense @mapleFU. I'm learning. :)

[felipecrv]

This LGTM. But what you think @bkietz?

[mapleFU]

@pitrou @bkietz would you mind take a look?

[pitrou]

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.

[mapleFU]

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

[mapleFU]

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

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9cafbb2.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.