waynexia commented on code in PR #6840:
URL: https://github.com/apache/arrow-datafusion/pull/6840#discussion_r1260704266
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -819,116 +840,38 @@ pub async fn from_substrait_rex(
),
})))
}
- Ok(ScalarFunctionType::Builtin(fun)) => {
-
Ok(Arc::new(Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![
- from_substrait_rex(l, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- from_substrait_rex(r, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- ],
- })))
- }
- Ok(ScalarFunctionType::Not) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Not".to_string(),
- ))
- }
- Err(e) => Err(e),
- }
- }
- (l, r) => Err(DataFusionError::NotImplemented(format!(
- "Invalid arguments for binary expression: {l:?} and {r:?}"
- ))),
- },
- // ScalarFunction or Expr::Not
- 1 => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => scalar_function_or_not(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Function not found: function reference = {:?}",
- f.function_reference
- ))),
- };
-
- match fun {
- Ok(ScalarFunctionType::Op(_)) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Op".to_string(),
- ))
- }
- Ok(scalar_function_type) => {
- match &f.arguments.first().unwrap().arg_type {
- Some(ArgType::Value(e)) => {
- let expr =
- from_substrait_rex(e, input_schema,
extensions)
- .await?
- .as_ref()
- .clone();
- match scalar_function_type {
- ScalarFunctionType::Builtin(fun) =>
Ok(Arc::new(
-
Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![expr],
- }),
- )),
- ScalarFunctionType::Not => {
- Ok(Arc::new(Expr::Not(Box::new(expr))))
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not expression"
- .to_string(),
- )),
- }
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not
expression".to_string(),
- )),
- }
+ (l, r) => Err(DataFusionError::NotImplemented(format!(
+ "Invalid arguments for binary expression: {l:?}
and {r:?}"
+ ))),
}
- Err(e) => Err(e),
}
- }
- // ScalarFunction
- _ => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => BuiltinScalarFunction::from_str(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Aggregated function not found: function reference =
{:?}",
- f.function_reference
- ))),
- };
-
- let mut args: Vec<Expr> = vec![];
- for arg in f.arguments.iter() {
+ ScalarFunctionType::Not => {
+ let arg = f.arguments.first().ok_or_else(|| {
Review Comment:
I'm not sure about this. In the previous code (like deserializing ReadRel)
we don't care about extra information from the proto. I.e., we only care about
if we can get the data necessary to construct our plan, and just ignore other
extra things. But on the other hand, redundant things sometimes indicate an
error or unexpected behavior (like finding a remaining screw after recovering
something 🫣).
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -819,116 +840,38 @@ pub async fn from_substrait_rex(
),
})))
}
- Ok(ScalarFunctionType::Builtin(fun)) => {
-
Ok(Arc::new(Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![
- from_substrait_rex(l, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- from_substrait_rex(r, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- ],
- })))
- }
- Ok(ScalarFunctionType::Not) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Not".to_string(),
- ))
- }
- Err(e) => Err(e),
- }
- }
- (l, r) => Err(DataFusionError::NotImplemented(format!(
- "Invalid arguments for binary expression: {l:?} and {r:?}"
- ))),
- },
- // ScalarFunction or Expr::Not
- 1 => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => scalar_function_or_not(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Function not found: function reference = {:?}",
- f.function_reference
- ))),
- };
-
- match fun {
- Ok(ScalarFunctionType::Op(_)) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Op".to_string(),
- ))
- }
- Ok(scalar_function_type) => {
- match &f.arguments.first().unwrap().arg_type {
- Some(ArgType::Value(e)) => {
- let expr =
- from_substrait_rex(e, input_schema,
extensions)
- .await?
- .as_ref()
- .clone();
- match scalar_function_type {
- ScalarFunctionType::Builtin(fun) =>
Ok(Arc::new(
-
Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![expr],
- }),
- )),
- ScalarFunctionType::Not => {
- Ok(Arc::new(Expr::Not(Box::new(expr))))
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not expression"
- .to_string(),
- )),
- }
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not
expression".to_string(),
- )),
- }
+ (l, r) => Err(DataFusionError::NotImplemented(format!(
+ "Invalid arguments for binary expression: {l:?}
and {r:?}"
+ ))),
}
- Err(e) => Err(e),
}
- }
- // ScalarFunction
- _ => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => BuiltinScalarFunction::from_str(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Aggregated function not found: function reference =
{:?}",
- f.function_reference
- ))),
- };
-
- let mut args: Vec<Expr> = vec![];
- for arg in f.arguments.iter() {
+ ScalarFunctionType::Not => {
+ let arg = f.arguments.first().ok_or_else(|| {
Review Comment:
I'm not sure about this. In the previous code (like deserializing ReadRel)
we don't care about extra information from the proto. I.e., we only care about
if we can get the data necessary to construct our plan, and just ignore other
extra things. But on the other hand, redundant things sometimes indicate an
error or unexpected behavior (like finding a remaining screw after recovering
something 🫣).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
