[CALCITE-3679] Allow lambda expressions in SQL queries #1733
Conversation
|
This PR is incomplete in terms of functionality and requires alot of refactoring and test cases addition. |
core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
Show resolved
Hide resolved
|
An interesting topic, would review if i have time ~ |
ritesh-kapoor
force-pushed
the
CALCITE-3679
branch
from
319e871
to
63d03a1
Compare
|
Need help in PR review :) |
|
@danny0405 Need you favour for PR review, I know its hard for you to review so many PR's. It would be really nice if you could help me with this one. :) |
ritesh-kapoor
force-pushed
the
CALCITE-3679
branch
from
728b38e
to
1f466c6
Compare
Would review this weekend ~ There are some conflicts, you can resolve them first. |
…bda parameters in expression, adding docs, fixing rex translation for lambda, adding map_filter method
ritesh-kapoor
force-pushed
the
CALCITE-3679
branch
from
1f466c6
to
6f55704
Compare
I have resolved conflicts, Thanks for taking out time. :) |
| if (argFamilies.get(i) != SqlTypeFamily.ANY) { | ||
| type = argFamilies.get(i) | ||
| .getDefaultConcreteType(callBinding.getTypeFactory()); | ||
| } |
There was a problem hiding this comment.
The type inference should happen in SqlLambdaOperator, not the type checker(only check validated node types).
There was a problem hiding this comment.
Type inference is done in SqlLambdaOperator because return and parameter type information has scope in LambdaOperandTypeChecker.
| @@ -687,6 +690,10 @@ private Expression translate0(RexNode expr, RexImpTable.NullAs nullAs, | |||
| Expression input = list.append("inp" + index + "_", x); // safe to share | |||
| return handleNullUnboxingIfNecessary(input, nullAs, storageType); | |||
| } | |||
| case LAMBDA_REF: { | |||
| final String name = ((RexLambdaRef) expr).getName(); | |||
| return Expressions.parameter(typeFactory.getJavaClass(expr.getType()), name); | |||
There was a problem hiding this comment.
I remove this and the tests still passes, so, it seems not necessary.
There was a problem hiding this comment.
This is important and there are test failures
FAILURE 49.5sec, 277 completed, 1 failed, 1 skipped, org.apache.calcite.test.CalciteSqlOperatorTest FAILURE 83.9sec, 5925 completed, 3 failed, 104 skipped, Gradle Test Run :core:test
| * field a.</p> | ||
| */ | ||
| public class RexLambdaRef extends RexSlot { | ||
| //~ Static fields/initializers --------------------------------------------- |
There was a problem hiding this comment.
Isn't is just a RexLocalRef ?
There was a problem hiding this comment.
This is used to refer lambda ref, appears to be similar but are different
| public SqlNodeList getParameters() { | ||
| return parameters; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unparse the SqlLambda correctly.
There was a problem hiding this comment.
Seems like Sql Lambda is unparsed correctly, Please refer SqlParserTest.testLambdaExpr test case
|
|
||
| @Override public RelDataType deriveType( | ||
| SqlValidator validator, SqlValidatorScope scope, SqlCall call) { | ||
| return validateOperands(validator, scope, call); |
There was a problem hiding this comment.
Should implement the checkOperandTypes correctly to deduce the type, you can take SqlCaseOperator for a reference.
There was a problem hiding this comment.
SqlCaseOperator has different implementation Lambda type inference is derived from LambdaOperandTypeChecker.java
| OperandTypes.sequence("MAP_FILTER(<ANY>, <LAMBDA(BOOLEAN, ANY, ANY)>)", | ||
| OperandTypes.family(SqlTypeFamily.MAP), | ||
| OperandTypes.lambda(SqlTypeFamily.BOOLEAN, SqlTypeFamily.ANY, SqlTypeFamily.ANY)), | ||
| SqlFunctionCategory.SYSTEM); |
There was a problem hiding this comment.
Shouldn't the operand type check be OperandTypes.family(SqlTypeFamily.MAP, SqlTypeFamily.LAMBDA) ?
There was a problem hiding this comment.
The Map Function accepts lambda which returns boolean value and takes two arguments of any type. This seems to be correct.
|
I will resolve conflicts after PR tagged LGTM. Let me know if there are concerns :) |
|
@ritesh-kapoor @danny0405 Hello, was this PR ever approved, or what was blocking it? I am looking into lambda functions in SQL queries in Calcite. |
julianhyde
force-pushed
the
main
branch
2 times, most recently
from
8a5cf83
to
cf7f71b
Compare
|
any updates here? |
|
@mihaibudiu can you close this PR? CALCITE-3679 got resolved in #3502 |
|
Closing in favor of #3502. |
No description provided.