goldmedal commented on code in PR #16610:
URL: https://github.com/apache/datafusion/pull/16610#discussion_r2177369793
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -617,31 +617,55 @@ impl Unparser<'_> {
}
fn get_field_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
- if args.len() != 2 {
- return internal_err!("get_field must have exactly 2 arguments");
- }
-
- let mut id = match &args[0] {
- Expr::Column(col) => match self.col_to_sql(col)? {
- ast::Expr::Identifier(ident) => vec![ident],
- ast::Expr::CompoundIdentifier(idents) => idents,
- other => return internal_err!("expected col_to_sql to return
an Identifier or CompoundIdentifier, but received: {:?}", other),
- },
- _ => return internal_err!("get_field expects first argument to be
column, but received: {:?}", &args[0]),
- };
-
- let field = match &args[1] {
- Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ match &args[0] {
+ Expr::Column(col) => {
+ let mut id = match self.col_to_sql(col)? {
+ ast::Expr::Identifier(ident) => vec![ident],
+ ast::Expr::CompoundIdentifier(idents) => idents,
+ other => return internal_err!("expected col_to_sql to
return an Identifier or CompoundIdentifier, but received: {:?}", other),
+ };
+ for arg in args.iter().skip(1) {
+ let field = match arg {
+ Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ _ => {
+ return internal_err!(
+ "get_field expects arguments except the first
one to be a string, but received: {:?}",
+ &arg
+ )
+ }
+ };
+ id.push(field);
+ }
Review Comment:
This for-loop isn't necessary, either. We can simplify it as above.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -617,31 +617,55 @@ impl Unparser<'_> {
}
fn get_field_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
- if args.len() != 2 {
- return internal_err!("get_field must have exactly 2 arguments");
- }
Review Comment:
I think we don't need to remove the condition. We still need to ensure the
length of arguments for `get_field` is 2.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -617,31 +617,55 @@ impl Unparser<'_> {
}
fn get_field_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
- if args.len() != 2 {
- return internal_err!("get_field must have exactly 2 arguments");
- }
-
- let mut id = match &args[0] {
- Expr::Column(col) => match self.col_to_sql(col)? {
- ast::Expr::Identifier(ident) => vec![ident],
- ast::Expr::CompoundIdentifier(idents) => idents,
- other => return internal_err!("expected col_to_sql to return
an Identifier or CompoundIdentifier, but received: {:?}", other),
- },
- _ => return internal_err!("get_field expects first argument to be
column, but received: {:?}", &args[0]),
- };
-
- let field = match &args[1] {
- Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ match &args[0] {
+ Expr::Column(col) => {
+ let mut id = match self.col_to_sql(col)? {
+ ast::Expr::Identifier(ident) => vec![ident],
+ ast::Expr::CompoundIdentifier(idents) => idents,
+ other => return internal_err!("expected col_to_sql to
return an Identifier or CompoundIdentifier, but received: {:?}", other),
+ };
+ for arg in args.iter().skip(1) {
+ let field = match arg {
+ Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ _ => {
+ return internal_err!(
+ "get_field expects arguments except the first
one to be a string, but received: {:?}",
+ &arg
+ )
+ }
+ };
+ id.push(field);
+ }
+ Ok(ast::Expr::CompoundIdentifier(id))
+ }
+ Expr::ScalarFunction(struct_expr) => {
+ let root = self
+ .scalar_function_to_sql(struct_expr.func.name(),
&struct_expr.args)?;
+ let mut access_chain = vec![];
+ for arg in args.iter().skip(1) {
+ let field = match arg {
+ Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ _ => {
+ return internal_err!(
+ "get_field expects arguments except the first
one to be a string, but received: {:?}",
+ &arg
+ )
+ }
+ };
+
access_chain.push(ast::AccessExpr::Dot(ast::Expr::Identifier(field)));
+ }
+ Ok(ast::Expr::CompoundFieldAccess {
+ root: Box::new(root),
+ access_chain,
+ })
Review Comment:
```suggestion
let field = match &args[1] {
Expr::Literal(lit, _) =>
self.new_ident_quoted_if_needs(lit.to_string()),
_ => {
return internal_err!(
"get_field expects arguments except the first
one to be a string, but received: {:?}",
&args[1]
)
}
};
Ok(ast::Expr::CompoundFieldAccess {
root: Box::new(root),
access_chain:
vec![ast::AccessExpr::Dot(ast::Expr::Identifier(field))],
})
```
We don't need this for-loop, actually. The arguments of `get_field` are
always `column_name` and `field_name`.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
