Skip to content

Commit 87c540e

Browse files
committed
feat(physical-expr-common): add proto helpers for variant match, outer node, required scalar field
Expressions migrating to `try_to_proto` / `try_from_proto` (#22418) keep hand-rolling three shapes that don't fit the existing `encode_child` / `decode_required_expression` helpers from #22513: - the outer `match &node.expr_type { ... }` that opens every `try_from_proto`, - the `PhysicalExprNode { expr_id: None, expr_type: Some(_) }` wrapper every built-in `try_to_proto` returns, and - the `ok_or_else(|| internal_datafusion_err!("X is missing required field 'Y'"))` for non-expression fields like `arrow_type` on `CastExpr` / `TryCastExpr`. This commit adds three thin helpers, all gated on `feature = "proto"`: - `expect_expr_variant!` macro — matches the outer `Option<ExprType>` and returns the inner payload (auto-derefs through `Box`), or returns an `internal_err!` naming the expected variant. - `proto_encode::expr_node` — constructs a `PhysicalExprNode` with no `expr_id` set from an `ExprType`. `expr_id` is a dedup tag used only by `DynamicFilterPhysicalExpr` (#22434); every other expression's `try_to_proto` reduces to `expr_node(ExprType::Foo(_))`. - `proto_decode::require_proto_field<T>` — mirrors `decode_required_expression` for non-`PhysicalExprNode` fields, keeping the "missing required field" message format in one place. No existing call sites are migrated in this commit; the follow-up ports the already-migrated expressions onto these helpers.
1 parent cab69a1 commit 87c540e

1 file changed

Lines changed: 188 additions & 0 deletions

File tree

datafusion/physical-expr-common/src/physical_expr.rs

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,31 @@ pub mod proto_encode {
576576
}
577577
}
578578

579+
/// Build a [`PhysicalExprNode`] with no `expr_id` set.
580+
///
581+
/// `expr_id` is a dedup tag used by node-sharing encoders (today only
582+
/// [`DynamicFilterPhysicalExpr`], see #22434); every other
583+
/// `PhysicalExpr` emits a node with `expr_id: None`. So
584+
/// `try_to_proto` for those reduces to:
585+
///
586+
/// ```ignore
587+
/// Ok(Some(expr_node(ExprType::Cast(Box::new(PhysicalCastNode { .. })))))
588+
/// ```
589+
///
590+
/// If your expression *does* participate in dedup, set
591+
/// [`PhysicalExprNode::expr_id`] on the returned value or construct the
592+
/// node literally.
593+
///
594+
/// [`DynamicFilterPhysicalExpr`]: https://docs.rs/datafusion-physical-expr/latest/datafusion_physical_expr/expressions/struct.DynamicFilterPhysicalExpr.html
595+
pub fn expr_node(
596+
expr_type: datafusion_proto_models::protobuf::physical_expr_node::ExprType,
597+
) -> PhysicalExprNode {
598+
PhysicalExprNode {
599+
expr_id: None,
600+
expr_type: Some(expr_type),
601+
}
602+
}
603+
579604
/// Internal dispatch trait. Implementors live in `datafusion-proto` and
580605
/// wrap the existing `PhysicalExtensionCodec` +
581606
/// `PhysicalProtoConverterExtension` plumbing. Expression authors should
@@ -619,6 +644,48 @@ pub mod proto_decode {
619644

620645
use super::PhysicalExpr;
621646

647+
/// Open the outer [`PhysicalExprNode`] and assert it carries the expected
648+
/// `ExprType` variant, returning the inner payload (auto-derefs through
649+
/// `Box`) or bailing with an `Internal` error.
650+
///
651+
/// Every `try_from_proto` starts with the same six-line `match`:
652+
///
653+
/// ```ignore
654+
/// let try_cast = match &node.expr_type {
655+
/// Some(protobuf::physical_expr_node::ExprType::TryCast(x)) => x.as_ref(),
656+
/// _ => return internal_err!("PhysicalExprNode is not a TryCastExpr"),
657+
/// };
658+
/// ```
659+
///
660+
/// With this macro that collapses to:
661+
///
662+
/// ```ignore
663+
/// let try_cast = expect_expr_variant!(
664+
/// node,
665+
/// protobuf::physical_expr_node::ExprType::TryCast,
666+
/// "TryCastExpr",
667+
/// );
668+
/// ```
669+
///
670+
/// Pass the variant as a `::` path so the macro stays agnostic to how
671+
/// the caller imports the proto types.
672+
#[macro_export]
673+
macro_rules! expect_expr_variant {
674+
($node:expr, $variant:path, $expr_name:literal $(,)?) => {{
675+
match &$node.expr_type {
676+
::core::option::Option::Some($variant(inner)) => inner,
677+
_ => {
678+
return ::datafusion_common::internal_err!(concat!(
679+
"PhysicalExprNode is not a ",
680+
$expr_name
681+
));
682+
}
683+
}
684+
}};
685+
}
686+
#[doc(inline)]
687+
pub use expect_expr_variant;
688+
622689
/// Decoder context handed to per-expression `try_from_proto` constructors.
623690
///
624691
/// Wraps an internal [`PhysicalExprDecode`] trait object plus a borrowed
@@ -693,6 +760,33 @@ pub mod proto_decode {
693760
}
694761
}
695762

763+
/// Unwrap a required non-expression proto field.
764+
///
765+
/// Mirrors [`PhysicalExprDecodeCtx::decode_required_expression`] for proto
766+
/// fields that aren't [`PhysicalExprNode`]s — e.g. the `arrow_type` of a
767+
/// `PhysicalCastNode` or the `scalar` of a `PhysicalLiteralNode`. Keeps
768+
/// the "missing required field" message format identical across
769+
/// expressions:
770+
///
771+
/// ```ignore
772+
/// let arrow_type = require_proto_field(
773+
/// cast_expr.arrow_type.as_ref(),
774+
/// "CastExpr",
775+
/// "arrow_type",
776+
/// )?;
777+
/// ```
778+
pub fn require_proto_field<T>(
779+
opt: Option<T>,
780+
expr_name: &str,
781+
field: &str,
782+
) -> Result<T> {
783+
opt.ok_or_else(|| {
784+
datafusion_common::internal_datafusion_err!(
785+
"{expr_name} is missing required field '{field}'"
786+
)
787+
})
788+
}
789+
696790
/// Internal dispatch trait. Implementors live in `datafusion-proto`.
697791
/// Expression authors should use [`PhysicalExprDecodeCtx`] instead of
698792
/// calling this directly.
@@ -1143,3 +1237,97 @@ mod test {
11431237
);
11441238
}
11451239
}
1240+
1241+
#[cfg(all(test, feature = "proto"))]
1242+
mod proto_helper_tests {
1243+
use datafusion_common::DataFusionError;
1244+
use datafusion_proto_models::protobuf::{
1245+
self, PhysicalColumn, PhysicalExprNode, physical_expr_node,
1246+
};
1247+
1248+
use crate::expect_expr_variant;
1249+
use crate::physical_expr::proto_decode::require_proto_field;
1250+
use crate::physical_expr::proto_encode::expr_node;
1251+
1252+
fn column_node() -> PhysicalExprNode {
1253+
PhysicalExprNode {
1254+
expr_id: None,
1255+
expr_type: Some(physical_expr_node::ExprType::Column(PhysicalColumn {
1256+
name: "a".to_string(),
1257+
index: 0,
1258+
})),
1259+
}
1260+
}
1261+
1262+
#[test]
1263+
fn expr_node_sets_no_expr_id() {
1264+
let node = expr_node(physical_expr_node::ExprType::Column(PhysicalColumn {
1265+
name: "a".to_string(),
1266+
index: 0,
1267+
}));
1268+
assert!(node.expr_id.is_none());
1269+
assert!(matches!(
1270+
node.expr_type,
1271+
Some(physical_expr_node::ExprType::Column(_))
1272+
));
1273+
}
1274+
1275+
#[test]
1276+
fn require_proto_field_returns_inner() {
1277+
let v = require_proto_field(Some(7_u32), "FooExpr", "answer").unwrap();
1278+
assert_eq!(v, 7);
1279+
}
1280+
1281+
#[test]
1282+
fn require_proto_field_reports_missing() {
1283+
let err = require_proto_field::<u32>(None, "FooExpr", "answer").unwrap_err();
1284+
assert!(matches!(
1285+
err,
1286+
DataFusionError::Internal(msg)
1287+
if msg.contains("FooExpr is missing required field 'answer'")
1288+
));
1289+
}
1290+
1291+
fn expect_column(
1292+
node: &PhysicalExprNode,
1293+
) -> Result<&PhysicalColumn, DataFusionError> {
1294+
let inner =
1295+
expect_expr_variant!(node, physical_expr_node::ExprType::Column, "Column",);
1296+
Ok(inner)
1297+
}
1298+
1299+
#[test]
1300+
fn expect_expr_variant_returns_inner_payload() {
1301+
let node = column_node();
1302+
let col = expect_column(&node).unwrap();
1303+
assert_eq!(col.name, "a");
1304+
}
1305+
1306+
#[test]
1307+
fn expect_expr_variant_rejects_wrong_variant() {
1308+
let node = PhysicalExprNode {
1309+
expr_id: None,
1310+
expr_type: Some(physical_expr_node::ExprType::Negative(Box::new(
1311+
protobuf::PhysicalNegativeNode { expr: None },
1312+
))),
1313+
};
1314+
let err = expect_column(&node).unwrap_err();
1315+
assert!(matches!(
1316+
err,
1317+
DataFusionError::Internal(msg) if msg.contains("PhysicalExprNode is not a Column")
1318+
));
1319+
}
1320+
1321+
#[test]
1322+
fn expect_expr_variant_rejects_missing_expr_type() {
1323+
let node = PhysicalExprNode {
1324+
expr_id: None,
1325+
expr_type: None,
1326+
};
1327+
let err = expect_column(&node).unwrap_err();
1328+
assert!(matches!(
1329+
err,
1330+
DataFusionError::Internal(msg) if msg.contains("PhysicalExprNode is not a Column")
1331+
));
1332+
}
1333+
}

0 commit comments

Comments
 (0)