Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-16 Thread via GitHub 


alamb commented on PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#issuecomment-2978146604

   Thanks again @jonathanc-n 


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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-16 Thread via GitHub 


alamb merged PR #16391:
URL: https://github.com/apache/datafusion/pull/16391


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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


jonathanc-n commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147461767


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",
+expr_index + 1,
+other,

Review Comment:
   fixed!



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


jonathanc-n commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147447123


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",
+expr_index + 1,
+other,

Review Comment:
   Yeah I think it would be, let me change that



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


comphead commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147446520


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",
+expr_index + 1,
+other,

Review Comment:
   yeah, I'm checking the message in the slt test
   ```
   got Utf8\("foo"\) \(Literal\(Utf8\("foo"\), None\)\)
   ```
   would be that self explanatory to keep the last one only? 



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


jonathanc-n commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147445833


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",
+expr_index + 1,
+other,

Review Comment:
   I think @alamb's intention here was to have one less verbose and one with 
more information in case the user wants. 



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


comphead commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147430655


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",
+expr_index + 1,
+other,

Review Comment:
   do we need to output `other` twice?



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-14 Thread via GitHub 


comphead commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2147430611


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,18 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_index, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {} ({:?})",

Review Comment:
   ```suggestion
   "Argument #{} must be an INTEGER or NULL, got {} 
({:?})",
   ```



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-13 Thread via GitHub 


jonathanc-n commented on PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#issuecomment-2971722156

   Fixed @alamb! Should be good now


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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-13 Thread via GitHub 


alamb commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2145088320


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,17 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_indice, expr) in exprs.iter().enumerate() {

Review Comment:
   I think `index` is a more standard singular form of `indices`
   
   so like
   
   ```suggestion
   for (expr_index, expr) in exprs.iter().enumerate() {
   ```



##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,17 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_indice, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {:?}",

Review Comment:
   I don' have any good suggestions
   
   I do think the `{:?}` might be hard to read for complex expressions. Maybe 
you could change this to have the display name instead. Something like
   
   ```suggestion
   "Argument #{} must be an integer literal or null 
value, got {} ({:?})", expr_index, other, other,
   ```



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

Re: [PR] fix: Fixed error handling for `generate_series/range` [datafusion]

2025-06-13 Thread via GitHub 


jonathanc-n commented on code in PR #16391:
URL: https://github.com/apache/datafusion/pull/16391#discussion_r2144430651


##
datafusion/functions-table/src/generate_series.rs:
##
@@ -197,11 +197,17 @@ impl TableFunctionImpl for GenerateSeriesFuncImpl {
 }
 
 let mut normalize_args = Vec::new();
-for expr in exprs {
+for (expr_indice, expr) in exprs.iter().enumerate() {
 match expr {
 Expr::Literal(ScalarValue::Null, _) => {}
 Expr::Literal(ScalarValue::Int64(Some(n)), _) => 
normalize_args.push(*n),
-_ => return plan_err!("First argument must be an integer 
literal"),
+other => {
+return plan_err!(
+"Argument #{} must be an integer literal or null 
value, got {:?}",

Review Comment:
   Is there a better way to word this, `Argument #...` seems awkward 🤷 



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