feat: new create_one ExpressionHandler API
#662
Conversation
|
note I'll be cleaning up/adding more tests. wanted to get some eyes on this approach first |
github-actions
bot
added
the
breaking-change
There was a problem hiding this comment.
One thing I want to consider/discuss about this approach is that we require that the expression struct heirarchy matches the schema one. So a schema Struct(Struct(Scalar(Int)) requires an expression Struct(Struct(Literal(int))). This code wouldn't allow a Literal(int) expression.
Idk if we want to enforce that requirement in the long run? It's very common for kernel to flatten out the fields of a schema (ex: in a visitor), so I don't see why we shouldn't allow flattened expressions.
Perhaps this acts as a safety thing. Kernel is the only one calling create_one, and it ensures that things are nested as we expected.
| let actual_rb: RecordBatch = actual | ||
| .into_any() | ||
| .downcast::<ArrowEngineData>() | ||
| .unwrap() | ||
| .into(); |
There was a problem hiding this comment.
might be worth having these lines, handler, and the create_one call in a helper function. Something like:
fn test_create_one(schema, expr) -> RecordBatches {
let handler = ArrowExpressionHandler;
let actual = handler.create_one(schema, &expr).unwrap();
actual
.into_any()
.downcast::<ArrowEngineData>()
.unwrap()
.into()
}| match expr { | ||
| // simple case: for literals, just create a single-row array and ensure the data types match | ||
| Expression::Literal(scalar) => { | ||
| let array = scalar.to_array(1)?; |
There was a problem hiding this comment.
We lose the nullability info in field.is_nullable(). Should we add a check:
if !field.is_nullable() && array.is_null(0) {
// return error
}| #[test] | ||
| fn test_create_one_string() { | ||
| let expr = Expression::struct_from([Expression::literal("a")]); | ||
| let schema = Arc::new(crate::schema::StructType::new([StructField::new( |
There was a problem hiding this comment.
Given we use schema::StructType several times, I think we should import it.
| Expression::literal(3), | ||
| ]); | ||
| let schema = Arc::new(crate::schema::StructType::new([ | ||
| StructField::new("a", DeltaDataTypes::INTEGER, true), |
There was a problem hiding this comment.
with a rebase, I think these can be moved over to StructField::nullable/StructField::not_null.
| let scalar = &self.scalars[self.next_scalar_idx]; | ||
| self.next_scalar_idx += 1; |
There was a problem hiding this comment.
Missing a type check?
let DataType::Primitive(scalar_type) = scalar.data_type() else { /* boom */ };
require!(scalar_type == ptype, /* boom */);| } | ||
|
|
||
| fn transform_struct_field(&mut self, field: &'a StructField) -> Option<Cow<'a, StructField>> { | ||
| self.recurse_into_struct_field(field) |
There was a problem hiding this comment.
Watch out -- struct field (or containing array/map) decides whether the child type is nullable
(any nullable ancestor makes the field nullable)
There was a problem hiding this comment.
still need to add this - will get to it soon
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #662 +/- ##
==========================================
+ Coverage 84.22% 84.28% +0.06%
==========================================
Files 77 77
Lines 17694 17959 +265
Branches 17694 17959 +265
==========================================
+ Hits 14902 15136 +234
- Misses 2080 2113 +33
+ Partials 712 710 -2 ☔ View full report in Codecov by Sentry. |
What changes are proposed in this pull request?
Adds a new
create_oneAPI for creating single-rowEngineDataby implementing aSchemaTransformto transform the given schema + leaf values into a single-rowArrowEngineDatafn create_oneto ourExpressionHandlertrait (breaking)create_oneforArrowExpressionHandlerThis PR affects the following public APIs
New
create_oneAPI required for ExpressionHandler. And added a newlen()method toStructType.How was this change tested?
Bunch of new unit tests.