Replace RecordBatch::with_schema_unchecked with RecordBatch::new_unchecked

[tustvold]

Which issue does this PR close?

Closes #.

Rationale for this change

These schema checks exist for a reason, it should not be possible to bypass them without using unsafe, as allows constructing invalid arrays, via conversion to StructArray, which in turn could easily lead to UB.

What changes are included in this PR?

Are there any user-facing changes?

No this method was added yesterday and has not been released - #7402

[tustvold]

Tbh I am pretty lukewarm on this API at all, if schema management is a hot path in your codebase something has gone very wrong... I figured making it unsafe would be less controversial than simply removing it, but that would be my preference...

[berkaysynnada]

Tbh I am pretty lukewarm on this API at all, if schema management is a hot path in your codebase something has gone very wrong... I figured making it unsafe would be less controversial than simply removing it, but that would be my preference...

I truly agree. This should be an unsafe method, at least

[alamb]

I agree this API seems awkward and not consistent with the other APIs in this crate

What I suggest is:

1. Remove this API
2. Add APIs for destructing a RecordBatch into its constituent parts:
3. Add an unsafe construction API (inline with the other parts of arrow)

So something like

impl RecordBatch {
  /// Creates a RecordBatch without any consistency checks
  pub unsafe fn new_unchecked(schema: Schema, arrays: vec<ArrayRef>, row_count) -> Self {
   ...
  }
  
  /// Return the inner fields of this RecordBatch, consuming sekf
  pub fn into_parts(self) -> (Schema, Vec<ArrayRef>, usize) {
   ...
  }
}

[andygrove]

For reference, there was a previous attempt to add something like this, but we were not able to merge it.

#6855

@comphead created his PR independently of my efforts, further demonstrating the need for some kind of solution in downstream projects.

[tustvold]

I read this as a performance optimisation, and therefore different from wanting to remove the constraints full stop as #6855, which was rejected for being unsound

[alamb]

In my opinion, if arrow users want to make RecordBatches and think they know when they can avoid the existing checks, offering an unsafe API to do it is the right thing to do. We should caveat it, etc, but at the end of the day I don't think we should be standing in the way of allowing people to make RecordBatches as they see fit

[tustvold]

Agreed, it is no different from people relying on undocumented behaviour, it's ultimately their choice to make. However, we should not compromise on soundness, and so any such API must be unsafe

[alamb]

BTW I also think we should add the following API. Can be a follow on

  /// Return the inner fields of this RecordBatch, consuming sekf
  pub fn into_parts(self) -> (Schema, Vec<ArrayRef>, usize) {
   ...
  }

[comphead]

Thanks @tustvold and @alamb for making it unsafe is awesome idea to double emphasize risks. Similar to https://doc.rust-lang.org/std/primitive.slice.html#method.get_unchecked where bounds checks are not done by STD lib and moved to user caller site instead

[tustvold]

As discussed in #7405 (comment) I have reverted with_schema_unchecked and added a new_unchecked constructor

[alamb]

Looks good to me

[tustvold]

FYI I also considered using RecordBatchOptions instead of just row_count, however, the only other option is irrelevant to an unchecked constructor - and is also something we probably should remove - #7406