You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "rrrokhtar (via GitHub)" <gi...@apache.org> on 2023/04/13 15:45:11 UTC
[GitHub] [age] rrrokhtar opened a new pull request, #807: [Mathematical Functions] Support factorial function
rrrokhtar opened a new pull request, #807:
URL: https://github.com/apache/age/pull/807
#### **Description:**
Added support for factorial function to age
#### **Project reference:**
https://github.com/orgs/apache/projects/237/views/1
#### **Demo:**
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] rrrokhtar closed pull request #807: [Mathematical Functions] Support factorial function
Posted by "rrrokhtar (via GitHub)" <gi...@apache.org>.
rrrokhtar closed pull request #807: [Mathematical Functions] Support factorial function
URL: https://github.com/apache/age/pull/807
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] rrrokhtar commented on pull request #807: [Mathematical Functions] Support factorial function
Posted by "rrrokhtar (via GitHub)" <gi...@apache.org>.
rrrokhtar commented on PR #807:
URL: https://github.com/apache/age/pull/807#issuecomment-1507753138
@jrgemignani
> One of the jobs of an SE is questioning whether something is wise to do, or not. But, let's suppose it is and go on to your next statement.
**Sometimes you need to proceed if you don't get the explanations/answers to your questions.**
> Technically, you can, although it is cumbersome -
```SQL
psql-11.5-5432-pgsql=# SELECT * from cypher('graph1', $$ RETURN pg_catalog.factorial(25::pg_bigint) $$) as (result numeric);
result
----------------------------
15511210043330985984000000
(1 row)
psql-11.5-5432-pgsql=#
```
Thanks for the explanation here I was not aware of needing to the type casting here.
>But sqrt follows the cypher specification that states sqrt outputs a float. There is no such specification for factorial as Neo4j doesn't have it. You need to be careful just copying another function without checking, or thinking about, how your functions' output should be represented and how the copied function works. Additionally, your function causes a loss of precision due to your variable usage.
**Is factorial not a standard outputs an integer? I am not sure what's the problem with the usage of integer type here? do you want to use it smth like big integer in postgresql?**
> Except that there are regression tests for all (unless we missed a few) supported mathematical functions. Here is sqrt's -
**Cool, you have a point**
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on pull request #807: [Mathematical Functions] Support factorial function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #807:
URL: https://github.com/apache/age/pull/807#issuecomment-1507741698
@rrrokhtar
> It is mentioned in the project that I am assigned to, so that creating it is not my suggestion at all but it is may be good to be added...
One of the jobs of an SE is questioning whether something is wise to do, or not. But, let's suppose it is and go on to your next statement.
> ...added because you can not use pg_catalog.factorial function inside the cypher query
Technically, you can, although it is cumbersome -
```
psql-11.5-5432-pgsql=# SELECT * from cypher('graph1', $$ RETURN pg_catalog.factorial(25::pg_bigint) $$) as (result numeric);
result
----------------------------
15511210043330985984000000
(1 row)
psql-11.5-5432-pgsql=#
```
> The variable types are following PostgreSQL and the similar mathematical functions on the same file like ag_sqrt.
But sqrt follows the **cypher specification** that states sqrt outputs a float. There is no such specification for factorial as Neo4j doesn't have it. You need to be careful just copying another function without checking, or thinking about, how your functions' output should be represented and how the copied function works. Additionally, your function causes a loss of precision due to your variable usage.
> There is no regression are added to the mathematical functions at all (i.e. age_sqrt) so I didn't write for it as well...
Except that there are regression tests for all (unless we missed a few) supported mathematical functions. Here is sqrt's -
```
--
-- sqrt()
--
SELECT * from cypher('expr', $$
RETURN sqrt(25)
$$) as (result agtype);
SELECT * from cypher('expr', $$
RETURN sqrt(1)
$$) as (result agtype);
SELECT * from cypher('expr', $$
RETURN sqrt(0)
$$) as (result agtype);
```
I was purposely vague in not pointing out where a lot of this stuff is because I feel that it would be good for you to look around and get better acquainted with AGE and the AGE repository before proceeding.
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] rrrokhtar commented on pull request #807: [Mathematical Functions] Support factorial function
Posted by "rrrokhtar (via GitHub)" <gi...@apache.org>.
rrrokhtar commented on PR #807:
URL: https://github.com/apache/age/pull/807#issuecomment-1507720576
**Thanks for your comment**
@jrgemignani Here are the answers to your questions
1. It is mentioned in the project that I am assigned to, so that creating it is not my suggestion at all but it is may be good to be added because you can not use pg_catalog.factorial function inside the cypher query, and if it is not good to have why do you think PostgreSQL has that https://www.postgresql.org/docs/11/functions-math.html
2. The variable types are following PostgreSQL and the similar mathematical functions on the same file like ag_sqrt.
3. There is no regression are added to the mathematical functions at all (i.e. age_sqrt) so I didn't write for it as well, if you like having tests for that I can do that no problem.
Let me know which parts need to be 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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on pull request #807: [Mathematical Functions] Support factorial function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #807:
URL: https://github.com/apache/age/pull/807#issuecomment-1507769517
@rrrokhtar Yeah, I get that. And I'm sorry if the person responsible for your project hasn't given you any direction or guidance yet. Hopefully, they will be more forthcoming in the future.
In C and in PostgreSQL there are different sizes of integers and floats, in AGTYPE there is just one of each. There are also different number types, like integer, float, and numeric. AGTYPE is structure with different types inside.
As you are outputting an AGTYPE integer, you should look at how that is internally represented and then make sure the whole process retains that precision, if at all possible.
Look at how PostgreSQL sets up the factorial function, its input datatypes, its output datatypes. Then think about how it should work in AGE, ask questions and get a consensus on your approach.
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on pull request #807: [Mathematical Functions] Support factorial function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #807:
URL: https://github.com/apache/age/pull/807#issuecomment-1507712147
@rrrokhtar Here are the problems that I have with this PR -
1. I'm not sure that factorial is a wise function to add to AGE as it easily overflows. Does Neo4j have it?
2. Your code limits its usefulness by using the wrong variable types and sizes.
3. While you have a nice graphic showing what it can do, you have zero regression tests written for it.
Please justify why this is needed, or is a good idea to have. If it is justified, please fix your code to make it more mathematically useful. And write a complete set of regression tests for all usages and edge 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.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org