[Variant] Avoid superflous validation checks

[friendlymatthew]

Which issue does this PR close?

• Closes [Variant] Remove superfluous check when validating monotonic offsets #7900

Rational for this change

We can avoid certain checks in the validation code since other checks already guarantee these invariants for us

[friendlymatthew]

cc @scovich @alamb

[alamb]

Thanks @friendlymatthew -- I have one question but otherwise looks great

[alamb]

I don't quite follow how this check ensures the offsets are monotonically increasing

Is it because slice_from_slice requires the bounds to be increasing?

[friendlymatthew]

Yes, this was my thinking

slice_from_slice will err if the slice range is invalid. A slice range is invalid when the start offset is greater than the end offset.

So when we iterate through the offsets, we build our slice range offsets[i]..offsets[i + 1]. If every slice attempt is successful, we can guarantee offsets are non-decreasing.

We still need to check if offsets[i] == offsets[i + 1] for any i. This is still a valid range and slice_from_slice will return a valid slice (empty bytes).

This is when the Variant::try_new_with_metadata will err.

 #[test]
fn foo() {
    let metadata = VariantMetadata::try_new(&[1, 0, 0]).unwrap();

    // the following will panic:
    // called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Received empty bytes")
    let v = Variant::try_new_with_metadata(metadata, &[]).unwrap();
}

[friendlymatthew]

Added the example above as a test case in a5a5e45

[alamb]

Thank you @friendlymatthew

[scovich]

not sure the extra let is helpful?

Suggested change

	let are_offsets_monotonic = offsets.is_sorted_by(|a, b| a < b);
	if !are_offsets_monotonic {
	if !offsets.is_sorted_by(|a, b| a < b) {

[scovich]

We only make a single pass now, so we no longer need to collect field ids into a vec. The only non-trivial tweak is to request the last field id specifically for the field id bounds check -- O(1) cost, so no need to materialize a whole just vec for that.

While you're at it, consider replacing the collect + is_sorted pair just below with just Iterator::is_sorted?

[friendlymatthew]

Hi, after we create an iterator of offsets, we immediately split into two different validation paths. I'm a bit unsure how to best handle this while avoiding the allocation.

I opened #7901 to track areas where we can avoid materialization.

[scovich]

Are we still tracking a TODO to eliminate this materialization?
Once the comment below is addressed, I think it's the only one left.

[friendlymatthew]

We are tracking #7901

[scovich]

LGTM, looking forward to the upcoming materialization fixes as well.

[alamb]

Thanks again @friendlymatthew and @scovich