You could imagine the cast dropping columns or adding columns of nulls in this case, too (ARROW-7051 would make that more efficient, and I believe this is needed to fully complete ARROW-14658)

Also, you can just return Status::TypeError(...) - no need to use the macro or construct the status manually.

So do you think 7051 is a pre-cursor to this change?

We can leave it for afterwards/we can file another JIRA for this case.

FWIW, instead of constructing dest2, why not just use arrow::struct_() to directly construct the type?

Now that I look at this, I think there's still a missing case here. We should copy the offset over as well (and then we should not slice the child arrays below), or if there's a top-level validity buffer, we should slice the buffer before copying. I don't think the tests notice this right now because there's no top-level validity buffer in any of the test cases.

StructArray::Make does take a null bitmap so we should be able to test this.

First part of this is done. For the sake of comprehensiveness I tried introducing null values into the main test, but didn't make a difference at all. Is that different from explicitly providing a bitmap as you've described?

It's different, yes, since StructArray is nested: for a given row, either the subarrays can be null, or the top-level StructArray itself may be null (and then the child array's nullity are ignored).

Great explanation - think I've added this to the test now but didn't change the pass/fail status. Still missing something?

This looks good now, since we're not slicing the child and copying over the out offset, so it should pass.

Though - sorry - but it's probably better to slice the children, not copy the offset, and slice the bitmap in in_array.buffers when copying? The reason being, if we Cast a large array that's been sliced, this right now will cast the entire child array, even though we only technically care about the sliced part. Does that make sense?

The null bitmap can be copied with CopyBitmap:

It would be something like

if (in_array.buffers[0]) {
  ARROW_ASSIGN_OR_RAISE(out_array->buffers[0], CopyBitmap(ctx->memory_pool(), in_array.buffers[0]->data(), in_array.offset, in_array.length));
}

Though, frankly, it should work below to set kernel.null_handling to INTERSECTION and then the compute infra will compute the null bitmap for you. (And you shouldn't have to mess with out_array->buffers or out_array->offset.)

Sorry, there's lots of knobs to turn for this, so there's not one obvious way to implement things. I would see if kernel.null_handling + slicing children before casting works (it should, as far as I can tell without building and trying it myself.)

No worries - this has been a great learning experience for me. Your code suggestion looks pretty similar to what's already being done with the List casts - I just didn't understand enough before why to include but makes sense here. Let me see how this all looks

nit, but we could factor out the checked_cast of the types here and below

This could be my lack of C++ knowledge but when I tried that I was running into an issue where StructType was being immediately deleted given its lack of move or copy operators. Can post the trace back later if helpful

What was tried? Something like this should work, off the top of my head:

const StructType& in_type = checked_cast<const StructType&>(*batch[0].type());

Yea pretty sure that was what I tried, but let me do that again tonight and post the traceback

Cool this is working. My mistake was trying to do const auto in_type = checked_cast<const StructType&>(*batch[0].type()); which yielded

/arrow/cpp/src/arrow/type.h:972:20: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = arrow::StructType::Impl; _Dp = std::default_delete<arrow::StructType::Impl>]’

Not using auto here works well