Replace ArrayDataBuilder with GenericListArray in ListArrayDecoder

[liamzwbao]

Which issue does this PR close?

• Part of [Epic] Replace ArrayData with direct Array construction, when possible #9298.

Rationale for this change

While implementing ListViewArrayDecoder in arrow-json, I noticed we could potentially retire ArrayDataBuilder inside ListArrayDecoder. Therefore, I'd like to use a small PR here to make sure there's no regression

What changes are included in this PR?

Replace ArrayDataBuilder with GenericListArray in ListArrayDecoder

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

[liamzwbao]

Hi @alamb @Jefffrey , would appreciate a review and a benchmark for this PR. Thanks!

[alamb]

Any particular benchmark you want me to run?

[alamb]

Thanks @liamzwbao -- this looks good to me -- I have just one question

[alamb]

Does try_new validate the offsets ? That could be a significant performance hit

Basically as long as this doesn't do additional validation I think it looks good to me

[liamzwbao]

Looks like it will do validation, but I think it's cheap and the local benchmark does not cause a regression. BTW, we don't have an unchecked API for ListArray, the new API simply unwrap the try_new.

arrow-rs/arrow-array/src/array/list_array.rs

Lines 205 to 251 in e4b68e6

	/// * `offsets.len() - 1 != nulls.len()`
	/// * `offsets.last() > values.len()`
	/// * `!field.is_nullable() && values.is_nullable()`
	/// * `field.data_type() != values.data_type()`
	pub fn try_new(
	field: FieldRef,
	offsets: OffsetBuffer<OffsetSize>,
	values: ArrayRef,
	nulls: Option<NullBuffer>,
	) -> Result<Self, ArrowError> {
	let len = offsets.len() - 1; // Offsets guaranteed to not be empty
	let end_offset = offsets.last().unwrap().as_usize();
	// don't need to check other values of `offsets` because they are checked
	// during construction of `OffsetBuffer`
	if end_offset > values.len() {
	return Err(ArrowError::InvalidArgumentError(format!(
	"Max offset of {end_offset} exceeds length of values {}",
	values.len()
	)));
	}
	if let Some(n) = nulls.as_ref() {
	if n.len() != len {
	return Err(ArrowError::InvalidArgumentError(format!(
	"Incorrect length of null buffer for {}ListArray, expected {len} got {}",
	OffsetSize::PREFIX,
	n.len(),
	)));
	}
	}
	if !field.is_nullable() && values.is_nullable() {
	return Err(ArrowError::InvalidArgumentError(format!(
	"Non-nullable field of {}ListArray {:?} cannot contain nulls",
	OffsetSize::PREFIX,
	field.name()
	)));
	}
	if field.data_type() != values.data_type() {
	return Err(ArrowError::InvalidArgumentError(format!(
	"{}ListArray expected data type {} got {} for {:?}",
	OffsetSize::PREFIX,
	field.data_type(),
	values.data_type(),
	field.name()
	)));
	}

[alamb]

I think this call simply creates an ArrayData (which is necessary given the API) so I am not sure it actually avoids any ArrayDatas

In order to avoid ArrayData we would probably need to change the signature of decode to return an ArrayRef directly (rather than ArrayData)

[liamzwbao]

Makes sense. I can change the signature of all the decoders if you think it's worth a try

[liamzwbao]

Any particular benchmark you want me to run?

This one should be good: https://github.com/apache/arrow-rs/blob/main/arrow-json/benches/json-reader.rs
I tested it locally (MacOS M3 MAX) and there's no regression. That's why I brought it up to see if we can get similar results on a cloud server

[liamzwbao]

Hi @alamb, just noticed that we don't have benchmark for List yet. I will create one and get back to you later