Honestly this seems quite complex / opaque to me -- I am not going to be eager to write unit tests in this fashion. I would personally rather keep things as simple and explicit as possible so it's obvious what is being tested.

I agree that this looks over-engineered for what it seems to be doing (I may be misunderstanding) -- basically generating test data and checking expected values.

I'll separate this into a separate PR, then. I think testing against random data has value, similar to using a fuzzer. Currently our random tests have a lot of boilerplate for generating the inputs and a lot of ad-hoc code for computing the expected values. IMHO it's worthwhile to have an interface for doing sanity checks across a wide swath of parameter space without needing to specify each of those manually (even if this is not enabled by default).

You are free to open a separate PR but I'm against this particular implementation of this approach to testing so unless something materially changes about the way the developer specifies the tests I'm likely to vote against the PR

https://issues.apache.org/jira/browse/ARROW-9135

@wesm if you have any ideas which would make this interface simpler, could you comment in the JIRA?

I'm not sure that the approach to testing is viable at all for this part of the project. The examples that were in this PR contained effectively reimplementations of the kernel logic in the property test specification. Let's look at Add for example

const auto& out_type = args[0]->type;

if (!args[0]->is_valid || !args[1]->is_valid) {
  return MakeNullScalar(out_type);
}

if (is_integer(out_type->id())) {
  ARROW_ASSIGN_OR_RAISE(auto lhs, Cast<UInt64Scalar>(args[0]));
  ARROW_ASSIGN_OR_RAISE(auto rhs, Cast<UInt64Scalar>(args[1]));
  return UInt64Scalar(lhs->value + rhs->value).CastTo(out_type);
}

if (is_floating(out_type->id())) {
  ARROW_ASSIGN_OR_RAISE(auto lhs, Cast<DoubleScalar>(args[0]));
  ARROW_ASSIGN_OR_RAISE(auto rhs, Cast<DoubleScalar>(args[1]));
  return DoubleScalar(lhs->value + rhs->value).CastTo(out_type);
}

return Status::NotImplemented("NYI");

I don't think it's a good idea to have a parallel collection of reimplementations of kernels.

I'm not sure what the name "property" is supposed to refer to.

https://en.wikipedia.org/wiki/Property_testing

Am I wrong, or is this generating a new test subclass for simply different parameter values? This does not seem sound, especially as it will blow up compile times.

It isn't generating new classes. TYPED_TEST_SUITE does this (one template instantiation per type) but INSTANTIATE_TEST_SUITE_P only iterates over the provided values (which in this case are all ScalarFunctionPropertyTestParam)

I agree that this looks over-engineered for what it seems to be doing (I may be misunderstanding) -- basically generating test data and checking expected values.

I don't think that the offset and length checks are needed anymore, you can either remove them or make them DCHECK

will do

As it happens this fails for sliced input. I'll restore the checks. For your interest, one approach i tried for is_valid was a MetaFunction which invoked a no-op ScalarFunction with INTERSECTION null handling then yanked the null bitmap from that. Unfortunately INTERSECTION doesn't currently support the zero copy case and I wasn't sure the approach would be acceptable but it did avoid repetition of null bitmap handling logic. What do you think?

Ah yes, indeed zero copy is not possible here in general. What do you think about doing the zero copy when arr.offset % 8 == 0? Then you can slice the bitmap.

PTAL

I don't know off the top of my head what will be the implications of modifying the offset of the output value, but I think it should be okay, and we can always fix it later if it becomes an issue

In general I'm not clear on what the kernel is/isn't allowed to modify in the out data. I've added can_write_into_slices=false , so IIUC the kernel should always exclusively own the out data.

It's not worth thinking too hard right now because kernel pipelining (temporary elision) is not implemented yet so until that happens it would be just speculation.