From 43c635740e5fe47d470a374b1dd93b58671b3890 Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Mon, 30 Jan 2023 17:27:00 +0800 Subject: [PATCH 1/6] replace &Option with Option<&T> --- .../proto/src/logical_plan/from_proto.rs | 66 +++++++++---------- .../proto/src/physical_plan/from_proto.rs | 31 +++++---- 2 files changed, 48 insertions(+), 49 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 6e772e7c84993..418130b03a3f6 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -876,7 +876,7 @@ pub fn parse_expr( .ok_or_else(|| Error::required("value"))? .try_into()?; - let expr = parse_required_expr(&field.expr, registry, "expr")?; + let expr = parse_required_expr(field.expr.as_ref(), registry, "expr")?; Ok(Expr::GetIndexedField(GetIndexedField::new( Box::new(expr), @@ -926,7 +926,7 @@ pub fn parse_expr( datafusion_expr::window_function::WindowFunction::AggregateFunction( aggr_function, ), - vec![parse_required_expr(&expr.expr, registry, "expr")?], + vec![parse_required_expr(expr.expr.as_ref(), registry, "expr")?], partition_by, order_by, window_frame, @@ -937,7 +937,7 @@ pub fn parse_expr( .ok_or_else(|| Error::unknown("BuiltInWindowFunction", *i))? .into(); - let args = parse_optional_expr(&expr.expr, registry)? + let args = parse_optional_expr(expr.expr.as_ref(), registry)? .map(|e| vec![e]) .unwrap_or_else(Vec::new); @@ -963,64 +963,64 @@ pub fn parse_expr( .map(|e| parse_expr(e, registry)) .collect::, _>>()?, expr.distinct, - parse_optional_expr(&expr.filter, registry)?.map(Box::new), + parse_optional_expr(expr.filter.as_ref(), registry)?.map(Box::new), ))) } ExprType::Alias(alias) => Ok(Expr::Alias( - Box::new(parse_required_expr(&alias.expr, registry, "expr")?), + Box::new(parse_required_expr(alias.expr.as_ref(), registry, "expr")?), alias.alias.clone(), )), ExprType::IsNullExpr(is_null) => Ok(Expr::IsNull(Box::new(parse_required_expr( - &is_null.expr, + is_null.expr.as_ref(), registry, "expr", )?))), ExprType::IsNotNullExpr(is_not_null) => Ok(Expr::IsNotNull(Box::new( - parse_required_expr(&is_not_null.expr, registry, "expr")?, + parse_required_expr(is_not_null.expr.as_ref(), registry, "expr")?, ))), ExprType::NotExpr(not) => Ok(Expr::Not(Box::new(parse_required_expr( - ¬.expr, registry, "expr", + not.expr.as_ref(), registry, "expr", )?))), ExprType::IsTrue(msg) => Ok(Expr::IsTrue(Box::new(parse_required_expr( - &msg.expr, registry, "expr", + msg.expr.as_ref(), registry, "expr", )?))), ExprType::IsFalse(msg) => Ok(Expr::IsFalse(Box::new(parse_required_expr( - &msg.expr, registry, "expr", + msg.expr.as_ref(), registry, "expr", )?))), ExprType::IsUnknown(msg) => Ok(Expr::IsUnknown(Box::new(parse_required_expr( - &msg.expr, registry, "expr", + msg.expr.as_ref(), registry, "expr", )?))), ExprType::IsNotTrue(msg) => Ok(Expr::IsNotTrue(Box::new(parse_required_expr( - &msg.expr, registry, "expr", + msg.expr.as_ref(), registry, "expr", )?))), ExprType::IsNotFalse(msg) => Ok(Expr::IsNotFalse(Box::new(parse_required_expr( - &msg.expr, registry, "expr", + msg.expr.as_ref(), registry, "expr", )?))), ExprType::IsNotUnknown(msg) => Ok(Expr::IsNotUnknown(Box::new( - parse_required_expr(&msg.expr, registry, "expr")?, + parse_required_expr(msg.expr.as_ref(), registry, "expr")?, ))), ExprType::Between(between) => Ok(Expr::Between(Between::new( - Box::new(parse_required_expr(&between.expr, registry, "expr")?), + Box::new(parse_required_expr(between.expr.as_ref(), registry, "expr")?), between.negated, - Box::new(parse_required_expr(&between.low, registry, "expr")?), - Box::new(parse_required_expr(&between.high, registry, "expr")?), + Box::new(parse_required_expr(between.low.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(between.high.as_ref(), registry, "expr")?), ))), ExprType::Like(like) => Ok(Expr::Like(Like::new( like.negated, - Box::new(parse_required_expr(&like.expr, registry, "expr")?), - Box::new(parse_required_expr(&like.pattern, registry, "pattern")?), + Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), parse_escape_char(&like.escape_char)?, ))), ExprType::Ilike(like) => Ok(Expr::ILike(Like::new( like.negated, - Box::new(parse_required_expr(&like.expr, registry, "expr")?), - Box::new(parse_required_expr(&like.pattern, registry, "pattern")?), + Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), parse_escape_char(&like.escape_char)?, ))), ExprType::SimilarTo(like) => Ok(Expr::SimilarTo(Like::new( like.negated, - Box::new(parse_required_expr(&like.expr, registry, "expr")?), - Box::new(parse_required_expr(&like.pattern, registry, "pattern")?), + Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), parse_escape_char(&like.escape_char)?, ))), ExprType::Case(case) => { @@ -1042,31 +1042,31 @@ pub fn parse_expr( }) .collect::, Box)>, Error>>()?; Ok(Expr::Case(Case::new( - parse_optional_expr(&case.expr, registry)?.map(Box::new), + parse_optional_expr(case.expr.as_ref(), registry)?.map(Box::new), when_then_expr, - parse_optional_expr(&case.else_expr, registry)?.map(Box::new), + parse_optional_expr(case.else_expr.as_ref(), registry)?.map(Box::new), ))) } ExprType::Cast(cast) => { - let expr = Box::new(parse_required_expr(&cast.expr, registry, "expr")?); + let expr = Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::Cast(Cast::new(expr, data_type))) } ExprType::TryCast(cast) => { - let expr = Box::new(parse_required_expr(&cast.expr, registry, "expr")?); + let expr = Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::TryCast(TryCast::new(expr, data_type))) } ExprType::Sort(sort) => Ok(Expr::Sort(Sort::new( - Box::new(parse_required_expr(&sort.expr, registry, "expr")?), + Box::new(parse_required_expr(sort.expr.as_ref(), registry, "expr")?), sort.asc, sort.nulls_first, ))), ExprType::Negative(negative) => Ok(Expr::Negative(Box::new( - parse_required_expr(&negative.expr, registry, "expr")?, + parse_required_expr(negative.expr.as_ref(), registry, "expr")?, ))), ExprType::InList(in_list) => Ok(Expr::InList { - expr: Box::new(parse_required_expr(&in_list.expr, registry, "expr")?), + expr: Box::new(parse_required_expr(in_list.expr.as_ref(), registry, "expr")?), list: in_list .list .iter() @@ -1295,7 +1295,7 @@ pub fn parse_expr( .iter() .map(|expr| parse_expr(expr, registry)) .collect::, Error>>()?, - filter: parse_optional_expr(&pb.filter, registry)?.map(Box::new), + filter: parse_optional_expr(pb.filter.as_ref(), registry)?.map(Box::new), }) } @@ -1389,7 +1389,7 @@ pub fn from_proto_binary_op(op: &str) -> Result { } fn parse_optional_expr( - p: &Option>, + p: Option<&Box>, registry: &dyn FunctionRegistry, ) -> Result, Error> { match p { @@ -1399,7 +1399,7 @@ fn parse_optional_expr( } fn parse_required_expr( - p: &Option>, + p: Option<&Box>, registry: &dyn FunctionRegistry, field: impl Into, ) -> Result { diff --git a/datafusion/proto/src/physical_plan/from_proto.rs b/datafusion/proto/src/physical_plan/from_proto.rs index f1f7f8844df20..98089813ee319 100644 --- a/datafusion/proto/src/physical_plan/from_proto.rs +++ b/datafusion/proto/src/physical_plan/from_proto.rs @@ -77,23 +77,23 @@ pub(crate) fn parse_physical_expr( ExprType::Literal(scalar) => Arc::new(Literal::new(scalar.try_into()?)), ExprType::BinaryExpr(binary_expr) => Arc::new(BinaryExpr::new( parse_required_physical_box_expr( - &binary_expr.l, + binary_expr.l.as_ref(), registry, "left", input_schema, )?, logical_plan::from_proto::from_proto_binary_op(&binary_expr.op)?, parse_required_physical_box_expr( - &binary_expr.r, + binary_expr.r.as_ref(), registry, "right", input_schema, )?, )), ExprType::DateTimeIntervalExpr(expr) => Arc::new(DateTimeIntervalExpr::try_new( - parse_required_physical_box_expr(&expr.l, registry, "left", input_schema)?, + parse_required_physical_box_expr(expr.l.as_ref(), registry, "left", input_schema)?, logical_plan::from_proto::from_proto_binary_op(&expr.op)?, - parse_required_physical_box_expr(&expr.r, registry, "right", input_schema)?, + parse_required_physical_box_expr(expr.r.as_ref(), registry, "right", input_schema)?, input_schema, )?), ExprType::AggregateExpr(_) => { @@ -112,22 +112,22 @@ pub(crate) fn parse_physical_expr( )); } ExprType::IsNullExpr(e) => Arc::new(IsNullExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, )), ExprType::IsNotNullExpr(e) => Arc::new(IsNotNullExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, )), ExprType::NotExpr(e) => Arc::new(NotExpr::new(parse_required_physical_box_expr( - &e.expr, + e.expr.as_ref(), registry, "expr", input_schema, )?)), ExprType::Negative(e) => Arc::new(NegativeExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, )), ExprType::InList(e) => Arc::new(InListExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, e.list .iter() .map(|x| parse_physical_expr(x, registry, input_schema)) @@ -165,12 +165,12 @@ pub(crate) fn parse_physical_expr( .transpose()?, )?), ExprType::Cast(e) => Arc::new(CastExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, convert_required!(e.arrow_type)?, DEFAULT_DATAFUSION_CAST_OPTIONS, )), ExprType::TryCast(e) => Arc::new(TryCastExpr::new( - parse_required_physical_box_expr(&e.expr, registry, "expr", input_schema)?, + parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, convert_required!(e.arrow_type)?, )), ExprType::ScalarFunction(e) => { @@ -222,13 +222,13 @@ pub(crate) fn parse_physical_expr( like_expr.negated, like_expr.case_insensitive, parse_required_physical_box_expr( - &like_expr.expr, + like_expr.expr.as_ref(), registry, "expr", input_schema, )?, parse_required_physical_box_expr( - &like_expr.pattern, + like_expr.pattern.as_ref(), registry, "pattern", input_schema, @@ -240,13 +240,12 @@ pub(crate) fn parse_physical_expr( } fn parse_required_physical_box_expr( - expr: &Option>, + expr: Option<&Box>, registry: &dyn FunctionRegistry, field: &str, input_schema: &Schema, ) -> Result, DataFusionError> { - expr.as_ref() - .map(|e| parse_physical_expr(e.as_ref(), registry, input_schema)) + expr.map(|e| parse_physical_expr(e.as_ref(), registry, input_schema)) .transpose()? .ok_or_else(|| { DataFusionError::Internal(format!("Missing required field {field:?}")) From 5de2534a0d0e2726d76150f63d134fb204996d03 Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Mon, 30 Jan 2023 17:45:29 +0800 Subject: [PATCH 2/6] fix fmt error --- .../proto/src/logical_plan/from_proto.rs | 66 ++++++++++++++---- .../proto/src/physical_plan/from_proto.rs | 68 +++++++++++++++---- 2 files changed, 106 insertions(+), 28 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 418130b03a3f6..514707b77e7f4 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -979,48 +979,80 @@ pub fn parse_expr( parse_required_expr(is_not_null.expr.as_ref(), registry, "expr")?, ))), ExprType::NotExpr(not) => Ok(Expr::Not(Box::new(parse_required_expr( - not.expr.as_ref(), registry, "expr", + not.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsTrue(msg) => Ok(Expr::IsTrue(Box::new(parse_required_expr( - msg.expr.as_ref(), registry, "expr", + msg.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsFalse(msg) => Ok(Expr::IsFalse(Box::new(parse_required_expr( - msg.expr.as_ref(), registry, "expr", + msg.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsUnknown(msg) => Ok(Expr::IsUnknown(Box::new(parse_required_expr( - msg.expr.as_ref(), registry, "expr", + msg.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsNotTrue(msg) => Ok(Expr::IsNotTrue(Box::new(parse_required_expr( - msg.expr.as_ref(), registry, "expr", + msg.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsNotFalse(msg) => Ok(Expr::IsNotFalse(Box::new(parse_required_expr( - msg.expr.as_ref(), registry, "expr", + msg.expr.as_ref(), + registry, + "expr", )?))), ExprType::IsNotUnknown(msg) => Ok(Expr::IsNotUnknown(Box::new( parse_required_expr(msg.expr.as_ref(), registry, "expr")?, ))), ExprType::Between(between) => Ok(Expr::Between(Between::new( - Box::new(parse_required_expr(between.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr( + between.expr.as_ref(), + registry, + "expr", + )?), between.negated, Box::new(parse_required_expr(between.low.as_ref(), registry, "expr")?), - Box::new(parse_required_expr(between.high.as_ref(), registry, "expr")?), + Box::new(parse_required_expr( + between.high.as_ref(), + registry, + "expr", + )?), ))), ExprType::Like(like) => Ok(Expr::Like(Like::new( like.negated, Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), - Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), + Box::new(parse_required_expr( + like.pattern.as_ref(), + registry, + "pattern", + )?), parse_escape_char(&like.escape_char)?, ))), ExprType::Ilike(like) => Ok(Expr::ILike(Like::new( like.negated, Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), - Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), + Box::new(parse_required_expr( + like.pattern.as_ref(), + registry, + "pattern", + )?), parse_escape_char(&like.escape_char)?, ))), ExprType::SimilarTo(like) => Ok(Expr::SimilarTo(Like::new( like.negated, Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), - Box::new(parse_required_expr(like.pattern.as_ref(), registry, "pattern")?), + Box::new(parse_required_expr( + like.pattern.as_ref(), + registry, + "pattern", + )?), parse_escape_char(&like.escape_char)?, ))), ExprType::Case(case) => { @@ -1048,12 +1080,14 @@ pub fn parse_expr( ))) } ExprType::Cast(cast) => { - let expr = Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); + let expr = + Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::Cast(Cast::new(expr, data_type))) } ExprType::TryCast(cast) => { - let expr = Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); + let expr = + Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::TryCast(TryCast::new(expr, data_type))) } @@ -1066,7 +1100,11 @@ pub fn parse_expr( parse_required_expr(negative.expr.as_ref(), registry, "expr")?, ))), ExprType::InList(in_list) => Ok(Expr::InList { - expr: Box::new(parse_required_expr(in_list.expr.as_ref(), registry, "expr")?), + expr: Box::new(parse_required_expr( + in_list.expr.as_ref(), + registry, + "expr", + )?), list: in_list .list .iter() diff --git a/datafusion/proto/src/physical_plan/from_proto.rs b/datafusion/proto/src/physical_plan/from_proto.rs index 98089813ee319..9a1db72ccb1b6 100644 --- a/datafusion/proto/src/physical_plan/from_proto.rs +++ b/datafusion/proto/src/physical_plan/from_proto.rs @@ -91,9 +91,19 @@ pub(crate) fn parse_physical_expr( )?, )), ExprType::DateTimeIntervalExpr(expr) => Arc::new(DateTimeIntervalExpr::try_new( - parse_required_physical_box_expr(expr.l.as_ref(), registry, "left", input_schema)?, + parse_required_physical_box_expr( + expr.l.as_ref(), + registry, + "left", + input_schema, + )?, logical_plan::from_proto::from_proto_binary_op(&expr.op)?, - parse_required_physical_box_expr(expr.r.as_ref(), registry, "right", input_schema)?, + parse_required_physical_box_expr( + expr.r.as_ref(), + registry, + "right", + input_schema, + )?, input_schema, )?), ExprType::AggregateExpr(_) => { @@ -111,23 +121,43 @@ pub(crate) fn parse_physical_expr( "Cannot convert sort expr node to physical expression".to_owned(), )); } - ExprType::IsNullExpr(e) => Arc::new(IsNullExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, - )), - ExprType::IsNotNullExpr(e) => Arc::new(IsNotNullExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, - )), + ExprType::IsNullExpr(e) => { + Arc::new(IsNullExpr::new(parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?)) + } + ExprType::IsNotNullExpr(e) => { + Arc::new(IsNotNullExpr::new(parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?)) + } ExprType::NotExpr(e) => Arc::new(NotExpr::new(parse_required_physical_box_expr( e.expr.as_ref(), registry, "expr", input_schema, )?)), - ExprType::Negative(e) => Arc::new(NegativeExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, - )), + ExprType::Negative(e) => { + Arc::new(NegativeExpr::new(parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?)) + } ExprType::InList(e) => Arc::new(InListExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, + parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?, e.list .iter() .map(|x| parse_physical_expr(x, registry, input_schema)) @@ -165,12 +195,22 @@ pub(crate) fn parse_physical_expr( .transpose()?, )?), ExprType::Cast(e) => Arc::new(CastExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, + parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?, convert_required!(e.arrow_type)?, DEFAULT_DATAFUSION_CAST_OPTIONS, )), ExprType::TryCast(e) => Arc::new(TryCastExpr::new( - parse_required_physical_box_expr(e.expr.as_ref(), registry, "expr", input_schema)?, + parse_required_physical_box_expr( + e.expr.as_ref(), + registry, + "expr", + input_schema, + )?, convert_required!(e.arrow_type)?, )), ExprType::ScalarFunction(e) => { From 132c69347801a05785fe560e13d6ee4369bbe25e Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Mon, 30 Jan 2023 21:21:36 +0800 Subject: [PATCH 3/6] fix clippy error --- .../proto/src/logical_plan/from_proto.rs | 66 +++++++++---------- .../proto/src/physical_plan/from_proto.rs | 28 ++++---- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 514707b77e7f4..04496832decdc 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -876,7 +876,7 @@ pub fn parse_expr( .ok_or_else(|| Error::required("value"))? .try_into()?; - let expr = parse_required_expr(field.expr.as_ref(), registry, "expr")?; + let expr = parse_required_expr(field.expr.clone(), registry, "expr")?; Ok(Expr::GetIndexedField(GetIndexedField::new( Box::new(expr), @@ -926,7 +926,7 @@ pub fn parse_expr( datafusion_expr::window_function::WindowFunction::AggregateFunction( aggr_function, ), - vec![parse_required_expr(expr.expr.as_ref(), registry, "expr")?], + vec![parse_required_expr(expr.expr.clone(), registry, "expr")?], partition_by, order_by, window_frame, @@ -937,7 +937,7 @@ pub fn parse_expr( .ok_or_else(|| Error::unknown("BuiltInWindowFunction", *i))? .into(); - let args = parse_optional_expr(expr.expr.as_ref(), registry)? + let args = parse_optional_expr(expr.expr.clone(), registry)? .map(|e| vec![e]) .unwrap_or_else(Vec::new); @@ -963,73 +963,73 @@ pub fn parse_expr( .map(|e| parse_expr(e, registry)) .collect::, _>>()?, expr.distinct, - parse_optional_expr(expr.filter.as_ref(), registry)?.map(Box::new), + parse_optional_expr(expr.filter.clone(), registry)?.map(Box::new), ))) } ExprType::Alias(alias) => Ok(Expr::Alias( - Box::new(parse_required_expr(alias.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(alias.expr.clone(), registry, "expr")?), alias.alias.clone(), )), ExprType::IsNullExpr(is_null) => Ok(Expr::IsNull(Box::new(parse_required_expr( - is_null.expr.as_ref(), + is_null.expr.clone(), registry, "expr", )?))), ExprType::IsNotNullExpr(is_not_null) => Ok(Expr::IsNotNull(Box::new( - parse_required_expr(is_not_null.expr.as_ref(), registry, "expr")?, + parse_required_expr(is_not_null.expr.clone(), registry, "expr")?, ))), ExprType::NotExpr(not) => Ok(Expr::Not(Box::new(parse_required_expr( - not.expr.as_ref(), + not.expr.clone(), registry, "expr", )?))), ExprType::IsTrue(msg) => Ok(Expr::IsTrue(Box::new(parse_required_expr( - msg.expr.as_ref(), + msg.expr.clone(), registry, "expr", )?))), ExprType::IsFalse(msg) => Ok(Expr::IsFalse(Box::new(parse_required_expr( - msg.expr.as_ref(), + msg.expr.clone(), registry, "expr", )?))), ExprType::IsUnknown(msg) => Ok(Expr::IsUnknown(Box::new(parse_required_expr( - msg.expr.as_ref(), + msg.expr.clone(), registry, "expr", )?))), ExprType::IsNotTrue(msg) => Ok(Expr::IsNotTrue(Box::new(parse_required_expr( - msg.expr.as_ref(), + msg.expr.clone(), registry, "expr", )?))), ExprType::IsNotFalse(msg) => Ok(Expr::IsNotFalse(Box::new(parse_required_expr( - msg.expr.as_ref(), + msg.expr.clone(), registry, "expr", )?))), ExprType::IsNotUnknown(msg) => Ok(Expr::IsNotUnknown(Box::new( - parse_required_expr(msg.expr.as_ref(), registry, "expr")?, + parse_required_expr(msg.expr.clone(), registry, "expr")?, ))), ExprType::Between(between) => Ok(Expr::Between(Between::new( Box::new(parse_required_expr( - between.expr.as_ref(), + between.expr.clone(), registry, "expr", )?), between.negated, - Box::new(parse_required_expr(between.low.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(between.low.clone(), registry, "expr")?), Box::new(parse_required_expr( - between.high.as_ref(), + between.high.clone(), registry, "expr", )?), ))), ExprType::Like(like) => Ok(Expr::Like(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.as_ref(), + like.pattern.clone(), registry, "pattern", )?), @@ -1037,9 +1037,9 @@ pub fn parse_expr( ))), ExprType::Ilike(like) => Ok(Expr::ILike(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.as_ref(), + like.pattern.clone(), registry, "pattern", )?), @@ -1047,9 +1047,9 @@ pub fn parse_expr( ))), ExprType::SimilarTo(like) => Ok(Expr::SimilarTo(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.as_ref(), + like.pattern.clone(), registry, "pattern", )?), @@ -1074,34 +1074,34 @@ pub fn parse_expr( }) .collect::, Box)>, Error>>()?; Ok(Expr::Case(Case::new( - parse_optional_expr(case.expr.as_ref(), registry)?.map(Box::new), + parse_optional_expr(case.expr.clone(), registry)?.map(Box::new), when_then_expr, - parse_optional_expr(case.else_expr.as_ref(), registry)?.map(Box::new), + parse_optional_expr(case.else_expr.clone(), registry)?.map(Box::new), ))) } ExprType::Cast(cast) => { let expr = - Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); + Box::new(parse_required_expr(cast.expr.clone(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::Cast(Cast::new(expr, data_type))) } ExprType::TryCast(cast) => { let expr = - Box::new(parse_required_expr(cast.expr.as_ref(), registry, "expr")?); + Box::new(parse_required_expr(cast.expr.clone(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::TryCast(TryCast::new(expr, data_type))) } ExprType::Sort(sort) => Ok(Expr::Sort(Sort::new( - Box::new(parse_required_expr(sort.expr.as_ref(), registry, "expr")?), + Box::new(parse_required_expr(sort.expr.clone(), registry, "expr")?), sort.asc, sort.nulls_first, ))), ExprType::Negative(negative) => Ok(Expr::Negative(Box::new( - parse_required_expr(negative.expr.as_ref(), registry, "expr")?, + parse_required_expr(negative.expr.clone(), registry, "expr")?, ))), ExprType::InList(in_list) => Ok(Expr::InList { expr: Box::new(parse_required_expr( - in_list.expr.as_ref(), + in_list.expr.clone(), registry, "expr", )?), @@ -1333,7 +1333,7 @@ pub fn parse_expr( .iter() .map(|expr| parse_expr(expr, registry)) .collect::, Error>>()?, - filter: parse_optional_expr(pb.filter.as_ref(), registry)?.map(Box::new), + filter: parse_optional_expr(pb.filter.clone(), registry)?.map(Box::new), }) } @@ -1427,7 +1427,7 @@ pub fn from_proto_binary_op(op: &str) -> Result { } fn parse_optional_expr( - p: Option<&Box>, + p: Option>, registry: &dyn FunctionRegistry, ) -> Result, Error> { match p { @@ -1437,7 +1437,7 @@ fn parse_optional_expr( } fn parse_required_expr( - p: Option<&Box>, + p: Option>, registry: &dyn FunctionRegistry, field: impl Into, ) -> Result { diff --git a/datafusion/proto/src/physical_plan/from_proto.rs b/datafusion/proto/src/physical_plan/from_proto.rs index 9a1db72ccb1b6..f7c4a794a5b42 100644 --- a/datafusion/proto/src/physical_plan/from_proto.rs +++ b/datafusion/proto/src/physical_plan/from_proto.rs @@ -77,14 +77,14 @@ pub(crate) fn parse_physical_expr( ExprType::Literal(scalar) => Arc::new(Literal::new(scalar.try_into()?)), ExprType::BinaryExpr(binary_expr) => Arc::new(BinaryExpr::new( parse_required_physical_box_expr( - binary_expr.l.as_ref(), + binary_expr.l.clone(), registry, "left", input_schema, )?, logical_plan::from_proto::from_proto_binary_op(&binary_expr.op)?, parse_required_physical_box_expr( - binary_expr.r.as_ref(), + binary_expr.r.clone(), registry, "right", input_schema, @@ -92,14 +92,14 @@ pub(crate) fn parse_physical_expr( )), ExprType::DateTimeIntervalExpr(expr) => Arc::new(DateTimeIntervalExpr::try_new( parse_required_physical_box_expr( - expr.l.as_ref(), + expr.l.clone(), registry, "left", input_schema, )?, logical_plan::from_proto::from_proto_binary_op(&expr.op)?, parse_required_physical_box_expr( - expr.r.as_ref(), + expr.r.clone(), registry, "right", input_schema, @@ -123,7 +123,7 @@ pub(crate) fn parse_physical_expr( } ExprType::IsNullExpr(e) => { Arc::new(IsNullExpr::new(parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, @@ -131,21 +131,21 @@ pub(crate) fn parse_physical_expr( } ExprType::IsNotNullExpr(e) => { Arc::new(IsNotNullExpr::new(parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, )?)) } ExprType::NotExpr(e) => Arc::new(NotExpr::new(parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, )?)), ExprType::Negative(e) => { Arc::new(NegativeExpr::new(parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, @@ -153,7 +153,7 @@ pub(crate) fn parse_physical_expr( } ExprType::InList(e) => Arc::new(InListExpr::new( parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, @@ -196,7 +196,7 @@ pub(crate) fn parse_physical_expr( )?), ExprType::Cast(e) => Arc::new(CastExpr::new( parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, @@ -206,7 +206,7 @@ pub(crate) fn parse_physical_expr( )), ExprType::TryCast(e) => Arc::new(TryCastExpr::new( parse_required_physical_box_expr( - e.expr.as_ref(), + e.expr.clone(), registry, "expr", input_schema, @@ -262,13 +262,13 @@ pub(crate) fn parse_physical_expr( like_expr.negated, like_expr.case_insensitive, parse_required_physical_box_expr( - like_expr.expr.as_ref(), + like_expr.expr.clone(), registry, "expr", input_schema, )?, parse_required_physical_box_expr( - like_expr.pattern.as_ref(), + like_expr.pattern.clone(), registry, "pattern", input_schema, @@ -280,7 +280,7 @@ pub(crate) fn parse_physical_expr( } fn parse_required_physical_box_expr( - expr: Option<&Box>, + expr: Option>, registry: &dyn FunctionRegistry, field: &str, input_schema: &Schema, From 8a7f04471580df91846366b2d8e6f43f2b5fb233 Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Mon, 30 Jan 2023 21:23:57 +0800 Subject: [PATCH 4/6] fix fmt error --- .../proto/src/logical_plan/from_proto.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 04496832decdc..74b9f67fc73af 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -1012,18 +1012,10 @@ pub fn parse_expr( parse_required_expr(msg.expr.clone(), registry, "expr")?, ))), ExprType::Between(between) => Ok(Expr::Between(Between::new( - Box::new(parse_required_expr( - between.expr.clone(), - registry, - "expr", - )?), + Box::new(parse_required_expr(between.expr.clone(), registry, "expr")?), between.negated, Box::new(parse_required_expr(between.low.clone(), registry, "expr")?), - Box::new(parse_required_expr( - between.high.clone(), - registry, - "expr", - )?), + Box::new(parse_required_expr(between.high.clone(), registry, "expr")?), ))), ExprType::Like(like) => Ok(Expr::Like(Like::new( like.negated, @@ -1100,11 +1092,7 @@ pub fn parse_expr( parse_required_expr(negative.expr.clone(), registry, "expr")?, ))), ExprType::InList(in_list) => Ok(Expr::InList { - expr: Box::new(parse_required_expr( - in_list.expr.clone(), - registry, - "expr", - )?), + expr: Box::new(parse_required_expr(in_list.expr.clone(), registry, "expr")?), list: in_list .list .iter() From 571e931c8d03f1217837cc75672d74af5f74cf32 Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Mon, 30 Jan 2023 23:08:06 +0800 Subject: [PATCH 5/6] replace Option<&Box> with Option<&> --- .../proto/src/physical_plan/from_proto.rs | 68 ++++++++----------- 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/datafusion/proto/src/physical_plan/from_proto.rs b/datafusion/proto/src/physical_plan/from_proto.rs index f7c4a794a5b42..d1141c850f9c8 100644 --- a/datafusion/proto/src/physical_plan/from_proto.rs +++ b/datafusion/proto/src/physical_plan/from_proto.rs @@ -76,30 +76,30 @@ pub(crate) fn parse_physical_expr( } ExprType::Literal(scalar) => Arc::new(Literal::new(scalar.try_into()?)), ExprType::BinaryExpr(binary_expr) => Arc::new(BinaryExpr::new( - parse_required_physical_box_expr( - binary_expr.l.clone(), + parse_required_physical_expr( + binary_expr.l.as_deref(), registry, "left", input_schema, )?, logical_plan::from_proto::from_proto_binary_op(&binary_expr.op)?, - parse_required_physical_box_expr( - binary_expr.r.clone(), + parse_required_physical_expr( + binary_expr.r.as_deref(), registry, "right", input_schema, )?, )), ExprType::DateTimeIntervalExpr(expr) => Arc::new(DateTimeIntervalExpr::try_new( - parse_required_physical_box_expr( - expr.l.clone(), + parse_required_physical_expr( + expr.l.as_deref(), registry, "left", input_schema, )?, logical_plan::from_proto::from_proto_binary_op(&expr.op)?, - parse_required_physical_box_expr( - expr.r.clone(), + parse_required_physical_expr( + expr.r.as_deref(), registry, "right", input_schema, @@ -122,38 +122,38 @@ pub(crate) fn parse_physical_expr( )); } ExprType::IsNullExpr(e) => { - Arc::new(IsNullExpr::new(parse_required_physical_box_expr( - e.expr.clone(), + Arc::new(IsNullExpr::new(parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, )?)) } ExprType::IsNotNullExpr(e) => { - Arc::new(IsNotNullExpr::new(parse_required_physical_box_expr( - e.expr.clone(), + Arc::new(IsNotNullExpr::new(parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, )?)) } - ExprType::NotExpr(e) => Arc::new(NotExpr::new(parse_required_physical_box_expr( - e.expr.clone(), + ExprType::NotExpr(e) => Arc::new(NotExpr::new(parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, )?)), ExprType::Negative(e) => { - Arc::new(NegativeExpr::new(parse_required_physical_box_expr( - e.expr.clone(), + Arc::new(NegativeExpr::new(parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, )?)) } ExprType::InList(e) => Arc::new(InListExpr::new( - parse_required_physical_box_expr( - e.expr.clone(), + parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, @@ -195,8 +195,8 @@ pub(crate) fn parse_physical_expr( .transpose()?, )?), ExprType::Cast(e) => Arc::new(CastExpr::new( - parse_required_physical_box_expr( - e.expr.clone(), + parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, @@ -205,8 +205,8 @@ pub(crate) fn parse_physical_expr( DEFAULT_DATAFUSION_CAST_OPTIONS, )), ExprType::TryCast(e) => Arc::new(TryCastExpr::new( - parse_required_physical_box_expr( - e.expr.clone(), + parse_required_physical_expr( + e.expr.as_deref(), registry, "expr", input_schema, @@ -261,14 +261,14 @@ pub(crate) fn parse_physical_expr( ExprType::LikeExpr(like_expr) => Arc::new(LikeExpr::new( like_expr.negated, like_expr.case_insensitive, - parse_required_physical_box_expr( - like_expr.expr.clone(), + parse_required_physical_expr( + like_expr.expr.as_deref(), registry, "expr", input_schema, )?, - parse_required_physical_box_expr( - like_expr.pattern.clone(), + parse_required_physical_expr( + like_expr.pattern.as_deref(), registry, "pattern", input_schema, @@ -279,27 +279,13 @@ pub(crate) fn parse_physical_expr( Ok(pexpr) } -fn parse_required_physical_box_expr( - expr: Option>, - registry: &dyn FunctionRegistry, - field: &str, - input_schema: &Schema, -) -> Result, DataFusionError> { - expr.map(|e| parse_physical_expr(e.as_ref(), registry, input_schema)) - .transpose()? - .ok_or_else(|| { - DataFusionError::Internal(format!("Missing required field {field:?}")) - }) -} - fn parse_required_physical_expr( expr: Option<&protobuf::PhysicalExprNode>, registry: &dyn FunctionRegistry, field: &str, input_schema: &Schema, ) -> Result, DataFusionError> { - expr.as_ref() - .map(|e| parse_physical_expr(e, registry, input_schema)) + expr.map(|e| parse_physical_expr(e, registry, input_schema)) .transpose()? .ok_or_else(|| { DataFusionError::Internal(format!("Missing required field {field:?}")) From 190077655c49176388f228d62fb191e4cab40432 Mon Sep 17 00:00:00 2001 From: gaoxinge Date: Tue, 31 Jan 2023 09:19:02 +0800 Subject: [PATCH 6/6] replace Option<&Box> with Option<&> --- .../proto/src/logical_plan/from_proto.rs | 112 +++++++++--------- 1 file changed, 58 insertions(+), 54 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 74b9f67fc73af..5aaf79997b638 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -876,7 +876,7 @@ pub fn parse_expr( .ok_or_else(|| Error::required("value"))? .try_into()?; - let expr = parse_required_expr(field.expr.clone(), registry, "expr")?; + let expr = parse_required_expr(field.expr.as_deref(), registry, "expr")?; Ok(Expr::GetIndexedField(GetIndexedField::new( Box::new(expr), @@ -926,7 +926,7 @@ pub fn parse_expr( datafusion_expr::window_function::WindowFunction::AggregateFunction( aggr_function, ), - vec![parse_required_expr(expr.expr.clone(), registry, "expr")?], + vec![parse_required_expr(expr.expr.as_deref(), registry, "expr")?], partition_by, order_by, window_frame, @@ -937,7 +937,7 @@ pub fn parse_expr( .ok_or_else(|| Error::unknown("BuiltInWindowFunction", *i))? .into(); - let args = parse_optional_expr(expr.expr.clone(), registry)? + let args = parse_optional_expr(expr.expr.as_deref(), registry)? .map(|e| vec![e]) .unwrap_or_else(Vec::new); @@ -963,65 +963,81 @@ pub fn parse_expr( .map(|e| parse_expr(e, registry)) .collect::, _>>()?, expr.distinct, - parse_optional_expr(expr.filter.clone(), registry)?.map(Box::new), + parse_optional_expr(expr.filter.as_deref(), registry)?.map(Box::new), ))) } ExprType::Alias(alias) => Ok(Expr::Alias( - Box::new(parse_required_expr(alias.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr( + alias.expr.as_deref(), + registry, + "expr", + )?), alias.alias.clone(), )), ExprType::IsNullExpr(is_null) => Ok(Expr::IsNull(Box::new(parse_required_expr( - is_null.expr.clone(), + is_null.expr.as_deref(), registry, "expr", )?))), ExprType::IsNotNullExpr(is_not_null) => Ok(Expr::IsNotNull(Box::new( - parse_required_expr(is_not_null.expr.clone(), registry, "expr")?, + parse_required_expr(is_not_null.expr.as_deref(), registry, "expr")?, ))), ExprType::NotExpr(not) => Ok(Expr::Not(Box::new(parse_required_expr( - not.expr.clone(), + not.expr.as_deref(), registry, "expr", )?))), ExprType::IsTrue(msg) => Ok(Expr::IsTrue(Box::new(parse_required_expr( - msg.expr.clone(), + msg.expr.as_deref(), registry, "expr", )?))), ExprType::IsFalse(msg) => Ok(Expr::IsFalse(Box::new(parse_required_expr( - msg.expr.clone(), + msg.expr.as_deref(), registry, "expr", )?))), ExprType::IsUnknown(msg) => Ok(Expr::IsUnknown(Box::new(parse_required_expr( - msg.expr.clone(), + msg.expr.as_deref(), registry, "expr", )?))), ExprType::IsNotTrue(msg) => Ok(Expr::IsNotTrue(Box::new(parse_required_expr( - msg.expr.clone(), + msg.expr.as_deref(), registry, "expr", )?))), ExprType::IsNotFalse(msg) => Ok(Expr::IsNotFalse(Box::new(parse_required_expr( - msg.expr.clone(), + msg.expr.as_deref(), registry, "expr", )?))), ExprType::IsNotUnknown(msg) => Ok(Expr::IsNotUnknown(Box::new( - parse_required_expr(msg.expr.clone(), registry, "expr")?, + parse_required_expr(msg.expr.as_deref(), registry, "expr")?, ))), ExprType::Between(between) => Ok(Expr::Between(Between::new( - Box::new(parse_required_expr(between.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr( + between.expr.as_deref(), + registry, + "expr", + )?), between.negated, - Box::new(parse_required_expr(between.low.clone(), registry, "expr")?), - Box::new(parse_required_expr(between.high.clone(), registry, "expr")?), + Box::new(parse_required_expr( + between.low.as_deref(), + registry, + "expr", + )?), + Box::new(parse_required_expr( + between.high.as_deref(), + registry, + "expr", + )?), ))), ExprType::Like(like) => Ok(Expr::Like(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.as_deref(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.clone(), + like.pattern.as_deref(), registry, "pattern", )?), @@ -1029,9 +1045,9 @@ pub fn parse_expr( ))), ExprType::Ilike(like) => Ok(Expr::ILike(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.as_deref(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.clone(), + like.pattern.as_deref(), registry, "pattern", )?), @@ -1039,9 +1055,9 @@ pub fn parse_expr( ))), ExprType::SimilarTo(like) => Ok(Expr::SimilarTo(Like::new( like.negated, - Box::new(parse_required_expr(like.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr(like.expr.as_deref(), registry, "expr")?), Box::new(parse_required_expr( - like.pattern.clone(), + like.pattern.as_deref(), registry, "pattern", )?), @@ -1052,47 +1068,45 @@ pub fn parse_expr( .when_then_expr .iter() .map(|e| { - let when_expr = parse_required_expr_inner( - e.when_expr.as_ref(), - registry, - "when_expr", - )?; - let then_expr = parse_required_expr_inner( - e.then_expr.as_ref(), - registry, - "then_expr", - )?; + let when_expr = + parse_required_expr(e.when_expr.as_ref(), registry, "when_expr")?; + let then_expr = + parse_required_expr(e.then_expr.as_ref(), registry, "then_expr")?; Ok((Box::new(when_expr), Box::new(then_expr))) }) .collect::, Box)>, Error>>()?; Ok(Expr::Case(Case::new( - parse_optional_expr(case.expr.clone(), registry)?.map(Box::new), + parse_optional_expr(case.expr.as_deref(), registry)?.map(Box::new), when_then_expr, - parse_optional_expr(case.else_expr.clone(), registry)?.map(Box::new), + parse_optional_expr(case.else_expr.as_deref(), registry)?.map(Box::new), ))) } ExprType::Cast(cast) => { let expr = - Box::new(parse_required_expr(cast.expr.clone(), registry, "expr")?); + Box::new(parse_required_expr(cast.expr.as_deref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::Cast(Cast::new(expr, data_type))) } ExprType::TryCast(cast) => { let expr = - Box::new(parse_required_expr(cast.expr.clone(), registry, "expr")?); + Box::new(parse_required_expr(cast.expr.as_deref(), registry, "expr")?); let data_type = cast.arrow_type.as_ref().required("arrow_type")?; Ok(Expr::TryCast(TryCast::new(expr, data_type))) } ExprType::Sort(sort) => Ok(Expr::Sort(Sort::new( - Box::new(parse_required_expr(sort.expr.clone(), registry, "expr")?), + Box::new(parse_required_expr(sort.expr.as_deref(), registry, "expr")?), sort.asc, sort.nulls_first, ))), ExprType::Negative(negative) => Ok(Expr::Negative(Box::new( - parse_required_expr(negative.expr.clone(), registry, "expr")?, + parse_required_expr(negative.expr.as_deref(), registry, "expr")?, ))), ExprType::InList(in_list) => Ok(Expr::InList { - expr: Box::new(parse_required_expr(in_list.expr.clone(), registry, "expr")?), + expr: Box::new(parse_required_expr( + in_list.expr.as_deref(), + registry, + "expr", + )?), list: in_list .list .iter() @@ -1321,7 +1335,8 @@ pub fn parse_expr( .iter() .map(|expr| parse_expr(expr, registry)) .collect::, Error>>()?, - filter: parse_optional_expr(pb.filter.clone(), registry)?.map(Box::new), + filter: parse_optional_expr(pb.filter.as_deref(), registry)? + .map(Box::new), }) } @@ -1415,27 +1430,16 @@ pub fn from_proto_binary_op(op: &str) -> Result { } fn parse_optional_expr( - p: Option>, + p: Option<&protobuf::LogicalExprNode>, registry: &dyn FunctionRegistry, ) -> Result, Error> { match p { - Some(expr) => parse_expr(expr.as_ref(), registry).map(Some), + Some(expr) => parse_expr(expr, registry).map(Some), None => Ok(None), } } fn parse_required_expr( - p: Option>, - registry: &dyn FunctionRegistry, - field: impl Into, -) -> Result { - match p { - Some(expr) => parse_expr(expr.as_ref(), registry), - None => Err(Error::required(field)), - } -} - -fn parse_required_expr_inner( p: Option<&protobuf::LogicalExprNode>, registry: &dyn FunctionRegistry, field: impl Into,