Dandandan commented on a change in pull request #519:
URL: https://github.com/apache/arrow-datafusion/pull/519#discussion_r646209257



##########
File path: datafusion/src/sql/utils.rs
##########
@@ -390,6 +391,30 @@ pub(crate) fn extract_aliases(exprs: &[Expr]) -> 
HashMap<String, Expr> {
         .collect::<HashMap<String, Expr>>()
 }
 
+pub(crate) fn resolve_positions_to_exprs(
+    expr: &Expr,
+    select_exprs: &[Expr],
+) -> Result<Expr> {
+    match expr {
+        Expr::Literal(position) => match position {
+            ScalarValue::Int64(Some(pos)) => {

Review comment:
       I think that's fine, its how the literals are created.
   So `Int64` is the only one we need to support!

##########
File path: datafusion/src/sql/utils.rs
##########
@@ -390,6 +391,30 @@ pub(crate) fn extract_aliases(exprs: &[Expr]) -> 
HashMap<String, Expr> {
         .collect::<HashMap<String, Expr>>()
 }
 
+pub(crate) fn resolve_positions_to_exprs(
+    expr: &Expr,
+    select_exprs: &[Expr],
+) -> Result<Expr> {
+    match expr {
+        Expr::Literal(position) => match position {

Review comment:
       ```suggestion
           Expr::Literal(position) if position > 0 && position < 
select_exprs.len() => match position {
   ```
    This way we check an extra possibility of error and this helps to reduce nr 
of branches.

##########
File path: datafusion/src/sql/utils.rs
##########
@@ -390,6 +391,29 @@ pub(crate) fn extract_aliases(exprs: &[Expr]) -> 
HashMap<String, Expr> {
         .collect::<HashMap<String, Expr>>()
 }
 
+pub(crate) fn resolve_positions_to_exprs(
+    expr: &Expr,
+    select_exprs: &[Expr],
+) -> Result<Expr> {
+    match expr {
+        // sql_expr_to_logical_expr maps number to i64
+        // 
https://github.com/apache/arrow-datafusion/blob/8d175c759e17190980f270b5894348dc4cff9bbf/datafusion/src/sql/planner.rs#L882-L887
+        Expr::Literal(ScalarValue::Int64(Some(pos))) => {

Review comment:
       We should also test whether `pos > 0` (otherwise `(pos - 1) as usize` 
will throw an exception in some cases. 




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to