You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "izveigor (via GitHub)" <gi...@apache.org> on 2023/04/28 07:23:17 UTC
[GitHub] [arrow-datafusion] izveigor opened a new pull request, #6149: feat: LCM, GCD and Factorial
izveigor opened a new pull request, #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/5838.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
Implementation "GCD", "LCM" and "Factorial" SQL functions.
# Are these changes tested?
Yes
# Are there any user-facing changes?
Yes
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on pull request #6149: feat: LCM, GCD and Factorial
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#issuecomment-1537206915
@alamb, could you review this PR if you have time.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on pull request #6149: feat: LCM, GCD and Factorial
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#issuecomment-1530038460
Hello, @alamb!
I noticed "Float64" problem, but I don't really understand how to solve it.
Do you have any idea what part of the code can cause this behavior?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6149: feat: LCM, GCD and Factorial
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#issuecomment-1530305491
> Hello, @alamb!
> I noticed "Float64" problem, but I don't really understand how to solve it.
> Do you have any idea what part of the code can cause this behavior?
Hi @izveigor -- I think what is happening is that the function's signature is (non obviously) being set to expect arguments via this fallback code:
https://github.com/apache/arrow-datafusion/blob/6e8f91b41a6ec6a2680357b95b2489d87af33571/datafusion/expr/src/function.rs#L677-L686
If you update `signature` to reflect the actual arguments desired (int64) I think the SQL will "just work"
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6149: feat: LCM, GCD and Factorial
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#discussion_r1181619630
##########
docs/source/user-guide/sql/scalar_functions.md:
##########
@@ -238,6 +241,19 @@ exp(numeric_expression)
- **numeric_expression**: Numeric expression to use as the exponent.
Can be a constant, column, or function, and any combination of arithmetic operators.
+### `factorial`
Review Comment:
❤️ for documentation
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #6149: feat: LCM, GCD and Factorial
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6149: feat: LCM, GCD and Factorial
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#discussion_r1181617813
##########
datafusion/physical-expr/src/math_expressions.rs:
##########
@@ -167,6 +168,98 @@ math_unary_function!("log10", log10);
math_unary_function!("degrees", to_degrees);
math_unary_function!("radians", to_radians);
+/// Factorial SQL function
+pub fn factorial(args: &[ArrayRef]) -> Result<ArrayRef> {
+ match args[0].data_type() {
+ DataType::UInt64 => Ok(Arc::new(make_function_scalar_inputs!(
+ &args[0],
+ "value",
+ UInt64Array,
+ { |value: u64| { (1..=value).product() } }
+ )) as ArrayRef),
+ other => Err(DataFusionError::Internal(format!(
+ "Unsupported data type {other:?} for function factorial"
Review Comment:
```suggestion
"Unsupported data type {other:?} for function factorial. Requires bigint"
```
I think it is nice to help the users understand what they can do to fix their query. The same applies to the queries below
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6149: feat: LCM, GCD and Factorial
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6149:
URL: https://github.com/apache/arrow-datafusion/pull/6149#issuecomment-1530306300
BTW I think the fact that the compiler didn't tell you where to update when you added a new function is confusing -- I will make a PR to fix 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org