feat: arrow convenience extensions

[roeap]

What changes are proposed in this pull request?

The PR introduces some convenience APIs for engines working with arrow data. Specifically we define and implement ScanExt and ExpressionEvaluatorExt which define variants of the main apis for Scan and ExpressionEvaluator respectively in terms of arrow RecordBatches.

PR #621 contains some similar work in defining a convenience function to handle Scan::execute results. In this PR a TryFrom impl is used - I was a bit unsure which approach would be better.

see: #826

also includes one cargo clippy.

This PR affects the following public APIs

new public methods when traits are in scope Scan::scan_metadata_arrow, Scan::evaluate_arrow and ExpressionEvaluator::evaluate_arrow.

How was this change tested?

additional unit tests for new APIs.

[codecov]

Codecov Report

❌ Patch coverage is 76.74419% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.99%. Comparing base (e74d18b) to head (873f7ce).
⚠️ Report is 136 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/arrow_extensions/scan.rs	78.94%	0 Missing and 8 partials ⚠️
kernel/src/engine/arrow_extensions/evaluator.rs	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #827      +/-   ##
==========================================
- Coverage   85.01%   84.99%   -0.02%     
==========================================
  Files          84       86       +2     
  Lines       20656    20699      +43     
  Branches    20656    20699      +43     
==========================================
+ Hits        17561    17594      +33     
- Misses       2228     2229       +1     
- Partials      867      876       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[scovich]

It would be nice if all the new extension methods had actual use sites, to give a better sense of how useful they are? Right now only execute_arrow has a real use site.

[scovich]

Why ?Sized? Are there dyn impl somewhere?

[scovich]

Or do we need that in order to invoke the associated function T::evaluate?

[scovich]

Is this a good use for Option::map_or_else?

Suggested change

	let record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	mask.map(|m| Ok(filter_record_batch(&record_batch, &m.into())?))
	.unwrap_or(Ok(record_batch))
	let record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	mask.map_or_else(
	|| Ok(record_batch),
	|m| Ok(filter_record_batch(&record_batch, &m.into())?),
	}

Tho simple imperative code probably wins on readability:

Suggested change

	let record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	mask.map(|m| Ok(filter_record_batch(&record_batch, &m.into())?))
	.unwrap_or(Ok(record_batch))
	let record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	Ok(match mask {
	Some(m) => filter_record_batch(&record_batch, &m.into())?,
	None => record_batch,
	})

or even

Suggested change

	let record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	mask.map(|m| Ok(filter_record_batch(&record_batch, &m.into())?))
	.unwrap_or(Ok(record_batch))
	let mut record_batch = ArrowEngineData::try_from_engine_data(data)?.into();
	if let Some(m) = mask {
	record_batch = filter_record_batch(&record_batch, &m.into())?;
	}
	Ok(record_batch)

[scovich]

IIRC, map_ok and flatten are a bad combination -- Err cases are silently dropped because they are treated as empty iterators. Does this work?

Suggested change

	.map_ok(TryFrom::try_from)
	.flatten())
	.map(|result| Ok(result?.try_into()?))
	.flatten_ok()

(depending on the error types, you might be able to drop the Ok(...?) wrapper)

(again below)

[roeap]

We currently maintain these in delta-rs, where they also support transitioning to kernel.

Once things stabelize we could revisit if we want to upstream those, but for now closing this here.