[jira] [Commented] (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 commented on CALCITE-3679: - 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] [Commented] (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=17786068#comment-17786068
]
Julian Hyde commented on CALCITE-3679:
--
I have reviewed [~hongyuguo]'s [PR
3502|https://github.com/apache/calcite/pull/3502] which fixes this case and
also CALCITE-6116. It looks good - my main requested changes are class names
(e.g. SqlLambdaExpression to SqlLambda) and visibility (e.g. removing
{{public}}).
The PR is in good shape. Nice work [~hongyuguo]!
My biggest concern is robustness of type-checking and validation.
Possibly the commit can be split into two - one adding lambdas, the other
adding the EXISTS function - but let's do that right before merge.
> 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.20.10#820010)
[jira] [Commented] (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=17703063#comment-17703063 ] jackylau commented on CALCITE-3679: --- How far along are we？ Does anyone make it one step further > 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.20.10#820010)
[jira] [Commented] (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=17212235#comment-17212235 ] Ritesh commented on CALCITE-3679: - Hey [~hyuan] I really wanted to work on this issue. But due to some personal reasons I might not be able to work on it this year. > 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] [Commented] (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=17211827#comment-17211827 ] Haisheng Yuan commented on CALCITE-3679: [~ritesh.kapoor] We appreciate your contribution and are looking forward to your update. Do you have any plan to continue working on this issue? > 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] [Commented] (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 commented on CALCITE-3679: - 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] [Commented] (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=17104108#comment-17104108 ] Julian Hyde commented on CALCITE-3679: -- Sorry my comments seemed harsh. Like you, I am a volunteer in this project, and have limited time to spend on each case that I review, so I'm sometimes not as diplomatic as I should be. Please don't be put off by my criticism. Re-reading what I wrote, most of it is constructive, and most of it is fair: * Yes, you did copy-paste comments without changing the text. As I said, it was my "first impression" but it was not incorrect. * 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. * 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 still believe that we need to get the type system sorted out early - i.e. in this PR. We should definitely have a conversation about what is "minimum viable product". Maybe this feature should be marked "experimental". I don't want to add major new features to Calcite, such as this, and then have to modify them in future releases. My review comments did not acknowledge the considerable amount of work you have done, and that was wrong. I should have acknowledged that. Also note that I was grumbling about the lambda implementations in Presto and Spark - not about your work. I have put considerable thought into lambdas in SQL - see my personal [Morel project|https://github.com/julianhyde/morel/] - so there is a good chance that we can do a better job than Presto or Spark did. 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. > 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] [Commented] (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 commented on CALCITE-3679:
-
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
> 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] [Commented] (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=17101070#comment-17101070 ] Julian Hyde commented on CALCITE-3679: -- 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. This case has no real description, so there's no indication of the intended scope. Do we intend to parse, validate, execute? 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. 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). > 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] [Commented] (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=17099514#comment-17099514 ] Ritesh commented on CALCITE-3679: - cc: [~zabetak] > 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] [Commented] (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=17099512#comment-17099512 ] Ritesh commented on CALCITE-3679: - It would be really great if this feature could make it to 1.23.0 release > 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] [Commented] (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=17058123#comment-17058123 ] Ritesh commented on CALCITE-3679: - Thanks [~Chunwei Lei], For taking out time to review this PR :) > 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: 2h 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)
[jira] [Commented] (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=17057538#comment-17057538 ] Chunwei Lei commented on CALCITE-3679: -- [~ritesh.kapoor] , thank you for your contribution. I would like to review it. > 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 50m > 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] [Commented] (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=17040591#comment-17040591 ] Ritesh commented on CALCITE-3679: - It would be really nice if we can rollout this feature in 1.22 release? > 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 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] [Commented] (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=17022766#comment-17022766 ] Ritesh commented on CALCITE-3679: - Variables are now resolved, Test cases added. Done with the changes. Need help in PR 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: Ritesh >Priority: Major > Labels: pull-request-available > Attachments: [CALCITE-3679]_Basic_implementation.patch > > Time Spent: 1.5h > 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] [Commented] (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 commented on CALCITE-3679: - 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)
