Skip to content

Commit c915742

Browse files
committed
Fix review findings in list_transform
Correctness: - A fallible body is no longer evaluated over element positions that no visible list references. ListView inputs (gaps, overlaps) and Lists with null rows (whose ranges Arrow permits to hold arbitrary values) now gather exactly the referenced elements into a compact List before applying the body. Infallible bodies keep the zero-cost structure-preserving path. - Zero-row batches short-circuit before the constant fast path, which could eagerly evaluate a fallible body over elements no row observes. - Fusion now collapses a whole transform chain in one rewrite (deep chains previously exhausted the optimizer's per-node iteration budget and errored on valid expressions) and skips bodies with more than one root reference (substitution previously duplicated the inner body per occurrence, compounding exponentially across fusion steps). - The identity rewrite moved to the typed simplify hook, gated on the input actually being list-typed, so ill-typed expressions still fail return_dtype instead of being rewritten into well-typed non-list ones. - serialize() now honors the vtable contract by returning Ok(None) when the body contains a non-serializable fn, via a new Expression::try_serialize_proto helper. Also: - The typed simplify hook optimizes the body against its element scope, since the options-embedded body is invisible to the optimizer's children traversal. - is_fallible uses a short-circuiting walk instead of a full label_tree labeling per call. - fmt_sql reuses the options Display impl so the lambda rendering lives in one place. - Regression tests for all of the above, including one pinning that the null-scalar branch protects fallible bodies from a null FixedSizeList constant's default-filled placeholder elements. - Test hygiene: imports at the top of the tests module, vortex_bail instead of panic, removed an assertion subsumed by assert_arrays_eq. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
1 parent 8d15d3a commit c915742

2 files changed

Lines changed: 369 additions & 42 deletions

File tree

vortex-array/src/expr/proto.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ use crate::scalar_fn::session::ScalarFnSessionExt;
1515
pub trait ExprSerializeProtoExt {
1616
/// Serialize the expression to its protobuf representation.
1717
fn serialize_proto(&self) -> VortexResult<pb::Expr>;
18+
19+
/// Serialize the expression to its protobuf representation, returning `Ok(None)` if any
20+
/// node in the tree is not serializable.
21+
fn try_serialize_proto(&self) -> VortexResult<Option<pb::Expr>>;
1822
}
1923

2024
impl ExprSerializeProtoExt for Expression {
@@ -35,6 +39,26 @@ impl ExprSerializeProtoExt for Expression {
3539
metadata: Some(metadata),
3640
})
3741
}
42+
43+
fn try_serialize_proto(&self) -> VortexResult<Option<pb::Expr>> {
44+
let mut children = Vec::with_capacity(self.children().len());
45+
for child in self.children().iter() {
46+
match child.try_serialize_proto()? {
47+
Some(child) => children.push(child),
48+
None => return Ok(None),
49+
}
50+
}
51+
52+
let Some(metadata) = self.options().serialize()? else {
53+
return Ok(None);
54+
};
55+
56+
Ok(Some(pb::Expr {
57+
id: self.id().to_string(),
58+
children,
59+
metadata: Some(metadata),
60+
}))
61+
}
3862
}
3963

4064
impl Expression {

0 commit comments

Comments
 (0)