Skip to content

Commit d1ec74e

Browse files
feat(physical-expr): port Literal to try_to_proto / try_from_proto hooks (apache#22636)
## Which issue does this PR close? Closes apache#22427 ## Rationale for this change `Literal` serialization/deserialization lived in the central downcast chains in `to_proto.rs` and `from_proto.rs`. This PR moves it into self-contained `try_to_proto` / `try_from_proto` hooks on `Literal` itself, following the pattern established for `NotExpr`, `NegativeExpr`, `IsNullExpr`, and `IsNotNullExpr`. ## What changes are included in this PR? - `datafusion/physical-expr/src/expressions/literal.rs` - Added `#[cfg(feature = "proto")] fn try_to_proto(...)` inside `impl PhysicalExpr for Literal` - Added `#[cfg(feature = "proto")] impl Literal { pub fn try_from_proto(...) }` - Added `proto_tests` module with encode, null-literal, roundtrip, and reject-wrong-variant tests - `datafusion/proto/src/physical_plan/to_proto.rs` - Removed the `Literal` downcast arm; removed `Literal` from import list - `datafusion/proto/src/physical_plan/from_proto.rs` - Replaced the inline `ExprType::Literal` arm with `Literal::try_from_proto(proto, &decode_ctx)?` ## Are there any user-facing changes? No. Serialization behaviour is identical; only the code location changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Kanishk Sachan <koopatroopa787>
1 parent c1f0d54 commit d1ec74e

3 files changed

Lines changed: 137 additions & 11 deletions

File tree

datafusion/physical-expr/src/expressions/literal.rs

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,41 @@ impl PhysicalExpr for Literal {
133133
fn placement(&self) -> ExpressionPlacement {
134134
ExpressionPlacement::Literal
135135
}
136+
137+
#[cfg(feature = "proto")]
138+
fn try_to_proto(
139+
&self,
140+
_ctx: &datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx<'_>,
141+
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
142+
use datafusion_proto_models::protobuf;
143+
144+
Ok(Some(protobuf::PhysicalExprNode {
145+
expr_id: None,
146+
expr_type: Some(protobuf::physical_expr_node::ExprType::Literal(
147+
(&self.value).try_into()?,
148+
)),
149+
}))
150+
}
151+
}
152+
153+
#[cfg(feature = "proto")]
154+
impl Literal {
155+
/// Reconstruct a [`Literal`] from its protobuf representation.
156+
pub fn try_from_proto(
157+
node: &datafusion_proto_models::protobuf::PhysicalExprNode,
158+
_ctx: &datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx<'_>,
159+
) -> Result<Arc<dyn PhysicalExpr>> {
160+
use datafusion_physical_expr_common::expect_expr_variant;
161+
use datafusion_proto_models::protobuf;
162+
163+
let scalar_proto = expect_expr_variant!(
164+
node,
165+
protobuf::physical_expr_node::ExprType::Literal,
166+
"Literal",
167+
);
168+
let value = ScalarValue::try_from(scalar_proto)?;
169+
Ok(Arc::new(Literal::new(value)))
170+
}
136171
}
137172

138173
/// Create a literal expression
@@ -190,3 +225,103 @@ mod tests {
190225
Ok(())
191226
}
192227
}
228+
229+
/// Tests for the `try_to_proto` / `try_from_proto` hooks.
230+
#[cfg(all(test, feature = "proto"))]
231+
mod proto_tests {
232+
use super::*;
233+
use crate::proto_test_util::{StubEncoder, UnreachableDecoder, column_node};
234+
use datafusion_common::DataFusionError;
235+
use datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx;
236+
use datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx;
237+
use datafusion_proto_models::protobuf::physical_expr_node;
238+
239+
fn i32_literal() -> Literal {
240+
Literal::new(ScalarValue::Int32(Some(42)))
241+
}
242+
243+
// ── try_to_proto ─────────────────────────────────────────────────────────
244+
245+
#[test]
246+
fn try_to_proto_encodes_literal() {
247+
let literal = i32_literal();
248+
let encoder = StubEncoder::ok();
249+
let ctx = PhysicalExprEncodeCtx::new(&encoder);
250+
251+
let node = literal
252+
.try_to_proto(&ctx)
253+
.unwrap()
254+
.expect("Literal should encode to Some(node)");
255+
256+
// Literal nodes never set expr_id.
257+
assert!(node.expr_id.is_none());
258+
// Variant must be Literal, not any other expr type.
259+
assert!(matches!(
260+
node.expr_type,
261+
Some(physical_expr_node::ExprType::Literal(_))
262+
));
263+
}
264+
265+
#[test]
266+
fn try_to_proto_null_literal() {
267+
let literal = Literal::new(ScalarValue::Int32(None));
268+
let encoder = StubEncoder::ok();
269+
let ctx = PhysicalExprEncodeCtx::new(&encoder);
270+
271+
let node = literal
272+
.try_to_proto(&ctx)
273+
.unwrap()
274+
.expect("null Literal should encode to Some(node)");
275+
276+
assert!(matches!(
277+
node.expr_type,
278+
Some(physical_expr_node::ExprType::Literal(_))
279+
));
280+
281+
// Decode and verify the null payload round-trips correctly.
282+
let schema = Schema::empty();
283+
let decoder = UnreachableDecoder;
284+
let dec_ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
285+
let decoded = Literal::try_from_proto(&node, &dec_ctx).unwrap();
286+
let lit = decoded
287+
.downcast_ref::<Literal>()
288+
.expect("decoded expr should be a Literal");
289+
assert_eq!(lit.value(), &ScalarValue::Int32(None));
290+
}
291+
292+
// ── try_from_proto ───────────────────────────────────────────────────────
293+
294+
#[test]
295+
fn try_from_proto_roundtrip() {
296+
let original = i32_literal();
297+
let encoder = StubEncoder::ok();
298+
let enc_ctx = PhysicalExprEncodeCtx::new(&encoder);
299+
300+
let node = original
301+
.try_to_proto(&enc_ctx)
302+
.unwrap()
303+
.expect("should encode");
304+
305+
let schema = Schema::empty();
306+
let decoder = UnreachableDecoder;
307+
let dec_ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
308+
309+
let decoded = Literal::try_from_proto(&node, &dec_ctx).unwrap();
310+
let lit = decoded
311+
.downcast_ref::<Literal>()
312+
.expect("decoded expr should be a Literal");
313+
assert_eq!(lit.value(), &ScalarValue::Int32(Some(42)));
314+
}
315+
316+
#[test]
317+
fn try_from_proto_rejects_non_literal_node() {
318+
let node = column_node("a");
319+
let schema = Schema::empty();
320+
let decoder = UnreachableDecoder;
321+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
322+
let err = Literal::try_from_proto(&node, &ctx).unwrap_err();
323+
assert!(
324+
matches!(err, DataFusionError::Internal(ref msg) if msg.contains("PhysicalExprNode is not a Literal"))
325+
);
326+
}
327+
}

datafusion/proto/src/physical_plan/from_proto.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,7 @@ pub fn parse_physical_expr_with_converter(
283283
// to the right constructor.
284284
ExprType::Column(_) => Column::try_from_proto(proto, &decode_ctx)?,
285285
ExprType::UnknownColumn(_) => UnKnownColumn::try_from_proto(proto, &decode_ctx)?,
286-
ExprType::Literal(scalar) => Arc::new(Literal::new(scalar.try_into()?)),
286+
ExprType::Literal(_) => Literal::try_from_proto(proto, &decode_ctx)?,
287287
ExprType::BinaryExpr(_) => BinaryExpr::try_from_proto(proto, &decode_ctx)?,
288288
ExprType::AggregateExpr(_) => {
289289
return not_impl_err!(

datafusion/proto/src/physical_plan/to_proto.rs

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,7 @@ use datafusion_physical_expr::ScalarFunctionExpr;
3535
use datafusion_physical_expr::scalar_subquery::ScalarSubqueryExpr;
3636
use datafusion_physical_expr::window::{SlidingAggregateWindowExpr, StandardWindowExpr};
3737
use datafusion_physical_expr_common::sort_expr::PhysicalSortExpr;
38-
use datafusion_physical_plan::expressions::{
39-
CaseExpr, DynamicFilterPhysicalExpr, Literal,
40-
};
38+
use datafusion_physical_plan::expressions::{CaseExpr, DynamicFilterPhysicalExpr};
4139
use datafusion_physical_plan::udaf::AggregateFunctionExpr;
4240
use datafusion_physical_plan::windows::{PlainAggregateWindowExpr, WindowUDFExpr};
4341
use datafusion_physical_plan::{
@@ -345,13 +343,6 @@ pub fn serialize_physical_expr_with_converter(
345343
),
346344
),
347345
})
348-
} else if let Some(lit) = expr.downcast_ref::<Literal>() {
349-
Ok(protobuf::PhysicalExprNode {
350-
expr_id,
351-
expr_type: Some(protobuf::physical_expr_node::ExprType::Literal(
352-
lit.value().try_into()?,
353-
)),
354-
})
355346
} else if let Some(expr) = expr.downcast_ref::<ScalarFunctionExpr>() {
356347
let mut buf = Vec::new();
357348
codec.try_encode_udf(expr.fun(), &mut buf)?;

0 commit comments

Comments
 (0)