[jira] [Comment Edited] (CALCITE-3679) Allow lambda expressions in SQL queries
[ https://issues.apache.org/jira/browse/CALCITE-3679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17798444#comment-17798444 ] hongyu guo edited comment on CALCITE-3679 at 12/19/23 6:33 AM: --- Fixed in [6f64865|https://github.com/apache/calcite/commit/6f64865eb8c71a65bde60a19de2de42775fae4ed]. [~ritesh.kapoor] Thanks for your contribution! [~julianhyde] [~mbudiu] Thanks for your review! was (Author: JIRAUSER300840): Solved in [6f64865|https://github.com/apache/calcite/commit/6f64865eb8c71a65bde60a19de2de42775fae4ed]. [~ritesh.kapoor] Thanks for your contribution! [~julianhyde] [~mbudiu] Thanks for your review! > Allow lambda expressions in SQL queries > --- > > Key: CALCITE-3679 > URL: https://issues.apache.org/jira/browse/CALCITE-3679 > Project: Calcite > Issue Type: New Feature >Reporter: Ritesh >Assignee: hongyu guo >Priority: Major > Labels: pull-request-available > Fix For: 1.37.0 > > Attachments: [CALCITE-3679]_Basic_implementation.patch > > Time Spent: 5h 40m > Remaining Estimate: 0h > > [https://teradata.github.io/presto/docs/0.167-t/functions/lambda.html] -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Comment Edited] (CALCITE-3679) Allow lambda expressions in SQL queries
[ https://issues.apache.org/jira/browse/CALCITE-3679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17109457#comment-17109457 ] Ritesh edited comment on CALCITE-3679 at 5/17/20, 12:38 PM: Yes, you did copy-paste comments without changing the text. As I said, it was my "first impression" but it was not incorrect. _*> Quoting my first comment in PR "This PR is incomplete in terms of functionality and requires alot of refactoring and test cases addition." I understand comments are important But I was more concerned with getting approach reviewed first, I will be more careful with the words. I will correct comments in PR :)*_ Yes, a bug does need a description. Especially when your only description is a link to a web page. (Which may change or disappear in future.) That web page has 7 functions and you only seem to have implemented 1. Implementing just one is fine, if you say you are implementing one. _+*> Agreed, I will update the description after closure on the scope.*+_ No, don't go tell me to read the PR to figure out the scope. You're just saying "it is what it is", which is not helpful. The scope needs to be described in the JIRA case. _*> I didn't mean to say "it is what it is", All I meant was if the scope is not clear it was better to be highlighted in the beginning, lets try not raise concern after 5 months of PR raised where alot of effort has already been invested.*_ I still believe that we need to get the type system sorted out early - i.e. in this PR. _*> Let me check on that and get back. :)*_ I also think that we can get this PR to a state where it can be committed. But realistically, 1.23 is too soon. It will take a few iterations. _*> Agreed, PR should be committed if it is in that state. I didn't intend to be pushy for 1.23 but I also don't want this PR to stay unmerged for long.*_ _*Let me update the PR with your feedback :)*_ was (Author: ritesh.kapoor): Yes, you did copy-paste comments without changing the text. As I said, it was my "first impression" but it was not incorrect. _*> Quoting my first comment in PR "This PR is incomplete in terms of functionality and requires alot of refactoring and test cases addition." I understand comments are important But I was more concerned with getting approach reviewed first, I will be more careful with the words. I will correct comments in PR :)*_ Yes, a bug does need a description. Especially when your only description is a link to a web page. (Which may change or disappear in future.) That web page has 7 functions and you only seem to have implemented 1. Implementing just one is fine, if you say you are implementing one. _+*> Agreed, I will update the description after closure on the scope.*+_ No, don't go tell me to read the PR to figure out the scope. You're just saying "it is what it is", which is not helpful. The scope needs to be described in the JIRA case. _*> I didn't mean to say "it is what it is", All I meant was if the scope is not clear it was better to be highlighted in the beginning, lets not raise concern after 5 months of PR raised where alot of effort has already been invested.*_ I still believe that we need to get the type system sorted out early - i.e. in this PR. _*> Let me check on that and get back. :)*_ I also think that we can get this PR to a state where it can be committed. But realistically, 1.23 is too soon. It will take a few iterations. _*> Agreed, PR should be committed if it is in that state. I didn't intend to be pushy for 1.23 but I also don't want this PR to stay unmerged for long.*_ _*Let me update the PR with your feedback :)*_ > Allow lambda expressions in SQL queries > --- > > Key: CALCITE-3679 > URL: https://issues.apache.org/jira/browse/CALCITE-3679 > Project: Calcite > Issue Type: New Feature >Reporter: Ritesh >Assignee: Ritesh >Priority: Major > Labels: pull-request-available > Attachments: [CALCITE-3679]_Basic_implementation.patch > > Time Spent: 5h 40m > Remaining Estimate: 0h > > [https://teradata.github.io/presto/docs/0.167-t/functions/lambda.html] -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Comment Edited] (CALCITE-3679) Allow lambda expressions in SQL queries
[
https://issues.apache.org/jira/browse/CALCITE-3679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17102257#comment-17102257
]
Ritesh edited comment on CALCITE-3679 at 5/8/20, 5:04 AM:
--
Hi [~julianhyde]
_*Your comments are highly de-motivating.*_
First impression is that the PR is rather scruffy. Comments have been
copy-pasted without being changed. Lines of whitespace have been added and
removed. Some classes have no javadoc.
Justify why we need a new class of RexSlot.
_*{color:#172b4d}There is no new class RexSlot and If you referring to
RexLambdaRef, I believe I have added comment in the PR itself.{color}*_
This case has no real description, so there's no indication of the intended
scope. Do we intend to parse, validate, execute?
_*If the intended scope was not clear It could have been really nice that this
should be pointed out in the beginning itself. Also I believe test cases are
self explanatory of what this PR is solving. Might have missed few comments but
thats what review is all about.*_
Lambdas in functional programming languages are functions-as-values. They are
typed, they allow closures (carrying their environment for later evaluation)
and they can be used anywhere that any other value is used. They are beautiful
and very powerful. So a half-hearted implementation would make me very sad.
_*All I wanted is to contribute, I tried to implement lambda as function
arguments, I asked for help alot many times to give me pointers to move ahead.
After investing alot of time myself came up with some solution & want community
to atleast review my approach.*_
The SQL implementations I have seen (Presto and SparkSQL) have a few built-in
higher-order functions (i.e. functions that take lambdas as arguments and/or
return lambdas as results) but do not allow user-defined higher-order
functions. This makes me sad.
A minimal level of functionality for Calcite would include support for lambdas
as standalone values (e.g. 'select empno, x -> x + 1 from emp') and adequate
typing of lambdas (not 'LAMBDA' but 'FUNCTION' or similar).
_*I have just started to contribute in this project and this was my first major
feature which I worked on, I couldn't develop full blown lambda functionality
without help. Releasing feature in phases is what I believe in.*_
_*I was just looking for guidance so that I could contribute. My efforts
counted as half hearted is really making me sad*_
was (Author: ritesh.kapoor):
Hi [~julianhyde]
_*Your comments are highly de-motivating.*_
First impression is that the PR is rather scruffy. Comments have been
copy-pasted without being changed. Lines of whitespace have been added and
removed. Some classes have no javadoc.
Justify why we need a new class of RexSlot.
_*{color:#172b4d}There is no new class RexSlot and If you referring to
RexLambdaRef, I believe I have added comment in the PR itself.{color}*_
This case has no real description, so there's no indication of the intended
scope. Do we intend to parse, validate, execute?
_*If the intended scope was not clear It could have been really nice that this
should be pointed out in the beginning itself. Also I believe test cases are
self explanatory of what this PR is solving. Might have missed few comments but
thats what review is all about.*_
Lambdas in functional programming languages are functions-as-values. They are
typed, they allow closures (carrying their environment for later evaluation)
and they can be used anywhere that any other value is used. They are beautiful
and very powerful. So a half-hearted implementation would make me very sad.
_*All I wanted is to contribute, I tried to implement lambda as function
arguments, I asked for help alot many times to give me pointers to move ahead.
After investing alot of time myself came up with some solution & want community
to atleast review my approach.*_
The SQL implementations I have seen (Presto and SparkSQL) have a few built-in
higher-order functions (i.e. functions that take lambdas as arguments and/or
return lambdas as results) but do not allow user-defined higher-order
functions. This makes me sad.
A minimal level of functionality for Calcite would include support for lambdas
as standalone values (e.g. 'select empno, x -> x + 1 from emp') and adequate
typing of lambdas (not 'LAMBDA' but 'FUNCTION' or similar).
_*I have just started to contribute in this project and this was my first major
feature which I worked on, I couldn't develop full blown lambda functionality
without help. Releasing feature in phases is what I believe in.*_
_*I was just looking for guidance so that I could contribute.*_
> Allow lambda expressions in SQL queries
> ---
>
> Key: CALCITE-3679
> URL: https://issues.apache.org/jira/browse/CALCITE-3679
> Project: Calcite
>
[jira] [Comment Edited] (CALCITE-3679) Allow lambda expressions in SQL queries
[
https://issues.apache.org/jira/browse/CALCITE-3679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17019561#comment-17019561
]
Ritesh edited comment on CALCITE-3679 at 1/20/20 3:49 PM:
--
Lambda expressions POC is mostly completed, Also, Able to resolve lambda
parameters in expression, But still unable to resolve parent scope variables in
expression. Any pointers would be helpful :)
Working example
{code:java}
map_filter(map[1, 2, 5, 4], (a,b)->a>b)
{code}
was (Author: ritesh.kapoor):
Lambda expressions POC is mostly completed, Able to resolve lambda parameters
in expression, But still unable to resolve parent scope variables in
expression. Any pointers would be helpful :)
> Allow lambda expressions in SQL queries
> ---
>
> Key: CALCITE-3679
> URL: https://issues.apache.org/jira/browse/CALCITE-3679
> Project: Calcite
> Issue Type: New Feature
>Reporter: Ritesh
>Assignee: Ritesh
>Priority: Major
> Labels: pull-request-available
> Attachments: [CALCITE-3679]_Basic_implementation.patch
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> [https://teradata.github.io/presto/docs/0.167-t/functions/lambda.html]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
