alamb commented on code in PR #10679:
URL: https://github.com/apache/datafusion/pull/10679#discussion_r1615939934
##########
datafusion/sql/src/utils.rs:
##########
@@ -149,47 +149,51 @@ pub(crate) fn extract_aliases(exprs: &[Expr]) ->
HashMap<String, Expr> {
}
/// Given an expression that's literal int encoding position, lookup the
corresponding expression
-/// in the select_exprs list, if the index is within the bounds and it is
indeed a position literal;
-/// Otherwise, return None
+/// in the select_exprs list, if the index is within the bounds and it is
indeed a position literal,
+/// otherwise, returns planning error.
+/// If input expression is not an int literal, returns expression as-is.
pub(crate) fn resolve_positions_to_exprs(
- expr: &Expr,
+ expr: Expr,
select_exprs: &[Expr],
-) -> Option<Expr> {
+) -> Result<Expr> {
match expr {
// sql_expr_to_logical_expr maps number to i64
//
https://github.com/apache/datafusion/blob/8d175c759e17190980f270b5894348dc4cff9bbf/datafusion/src/sql/planner.rs#L882-L887
Expr::Literal(ScalarValue::Int64(Some(position)))
- if position > &0_i64 && position <= &(select_exprs.len() as i64) =>
+ if position > 0_i64 && position <= select_exprs.len() as i64 =>
{
let index = (position - 1) as usize;
let select_expr = &select_exprs[index];
- Some(match select_expr {
+ Ok(match select_expr {
Expr::Alias(Alias { expr, .. }) => *expr.clone(),
_ => select_expr.clone(),
})
}
- _ => None,
+ Expr::Literal(ScalarValue::Int64(Some(position))) => plan_err!(
+ "Cannot find column with position {} in SELECT clause",
Review Comment:
The fact that these positions are 1 based sometimes is confusing for people
who might expect them to be zero based. I suggest we improve the message with
the valid values. For example:
```suggestion
"Cannot find column with position {} in SELECT clause. Valid
columns: 1 to {}",
select_exprs.len()
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
