Conversation
|
@b-slim Please let me know when this PR is ready to review again. |
|
@julianhyde i think the it is ready still can not figure out a way to ITest this because the Calcite Query execution does re-write the IN to an OR so can't get to the branch i want. Can you suggest a way to tweak the query engine used or disable that rule just for ITest ? |
|
Calcite If it's really important that you generate IN in Druid queries, you'll need to write some logic that converts ORs back to IN. But I wouldn't be surprised if Druid sargs its expressions. |
|
@julianhyde please checkout the UT |
| } | ||
| return new JsonInFilter("in", tr(e, posRef), listBuilder.build()); | ||
| case BETWEEN: | ||
| return new JsonBound("bound", tr(e, posRef), tr(e, posConstant), false, |
There was a problem hiding this comment.
I think this will fail, since BETWEEN first parameter is whether it is negated or not, then the reference, and then the bounds (thus, 4 arguments in total). You can check org.apache.calcite.sql.fun.SqlBetweenOperator.
| RexNode.class | ||
| ); | ||
| RexNode[] rexNodes = new RexNode[1]; | ||
| RexCall rexNode = Mockito.mock(RexCall.class); |
There was a problem hiding this comment.
I think that it is easier (and more reliable) to create the RexNode objects instead of mocking them (as in org.apache.calcite.rex.RexExecutorTest). For instance, if I am not mistaken, by calling rexBuilder.makeCall(SqlStdOperatorTable.BETWEEN, ..., the validator would have caught the issue with the BETWEEN operator.
There was a problem hiding this comment.
@jcamachor was able to do that for in filter but for between i get an exception. try to run this in your IDE
RexNode betweenRexNode = rexBuilder.makeCall(SqlStdOperatorTable.BETWEEN, Lists.<RexNode>newArrayList());i get a cast exception. @julianhyde any idea ? how to fix this ?
jcamachor
left a comment
jcamachor
left a comment
There was a problem hiding this comment.
Let me know if making those changes, BETWEEN operator works.
|
|
||
| final List<? extends RexNode> listRexNodes = Lists.newArrayList( | ||
| rexBuilder.makeInputRef(rowType, 0), | ||
| rexBuilder.makeLiteral("upper-bound"), |
There was a problem hiding this comment.
Use number for the bounds, as for IN clause.
| IOException { | ||
|
|
||
| final List<? extends RexNode> listRexNodes = Lists.newArrayList( | ||
| rexBuilder.makeInputRef(rowType, 0), |
There was a problem hiding this comment.
First argument, before the input ref, should be rexBuilder.makeLiteral(false) (it represents whether it is a negated BETWEEN or not).
There was a problem hiding this comment.
I wouldn't pass a boolean as an argument. I'd make it a call to SqlStdOperatorTable.NOT_BETWEEN. Or, better, don't allow NOT BETWEEN at all. Just use NOT(BETWEEN ...).
As you can see we're in uncharted territory because no one has ever created RexCalls on BETWEEN before.
There was a problem hiding this comment.
On reflection, the way we should be doing this is translating RexNode to SqlNode. (See SqlImplementor, used by the JDBC adapter and RelToSqlConverter.) SQL is a richer language than RexNode, and close enough to Druid's expression language.
It's too late to do that now, but we need to do much more work in this area, that's the approach I will be advocating.
|
@jcamachor thanks for the suggestions but still getting |
|
here is the stack java.lang.ClassCastException: org.apache.calcite.rex.RexCallBinding cannot be cast to org.apache.calcite.sql.SqlCallBinding
at org.apache.calcite.sql.fun.SqlBetweenOperator.inferReturnType(SqlBetweenOperator.java:139)
at org.apache.calcite.rex.RexBuilder.deriveReturnType(RexBuilder.java:272)
at org.apache.calcite.rex.RexBuilder.makeCall(RexBuilder.java:246)
at org.apache.calcite.adapter.druid.DruidQueryFilterTest.testBetweenFilter(DruidQueryFilterTest.java:119)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:237)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Process finished with exit code 255 |
|
@julianhyde any chance you can help with this ? |
|
@jcamachor thanks adding the type within the makeCall helped ! |
b-slim
force-pushed
the
CALCITE-1655-add_in_filter
branch
2 times, most recently
from
c5a7b80 to
0185802
Compare
|
@julianhyde and @jcamachor this is ready to be reviewed. |
Fix up We cannot do end-to-end tests of IN and BETWEEN because Calcite expands these before creating RexNode, but this change contains unit tests of converting RexCall with IN and BETWEEN into Druid JSON. Close apache#381
Fix up We cannot do end-to-end tests of IN and BETWEEN because Calcite expands these before creating RexNode, but this change contains unit tests of converting RexCall with IN and BETWEEN into Druid JSON. Close apache#381
|
@b-slim Did you see my comment in https://issues.apache.org/jira/browse/CALCITE-1655 about the test failure on JDK 9? |
|
@julianhyde i acknowledge that, i will investigate that, thanks ! |
b-slim
force-pushed
the
CALCITE-1655-add_in_filter
branch
from
0185802 to
86f49c2
Compare
|
@julianhyde please checkout this one. Looks like Mockito version is not compatible with JDK 9. |
|
Ah, I'd forgotten that Mockito was broken. I'll apply the same workaround as in https://issues.apache.org/jira/browse/CALCITE-1567. |
|
Cancel that. The fix is to upgrade Mockito, like we did in https://issues.apache.org/jira/browse/CALCITE-1568. |
Fix up We cannot do end-to-end tests of IN and BETWEEN because Calcite expands these before creating RexNode, but this change contains unit tests of converting RexCall with IN and BETWEEN into Druid JSON. Close apache#381
We cannot do end-to-end tests of IN and BETWEEN because Calcite expands these before creating RexNode, but this change contains unit tests of converting RexCall with IN and BETWEEN into Druid JSON. Close apache#381
apache#381) Signed-off-by: Mihai Budiu <mbudiu@feldera.com> Co-authored-by: Mihai Budiu <mbudiu@feldera.com>
apache#381) Signed-off-by: Mihai Budiu <mbudiu@feldera.com> Co-authored-by: Mihai Budiu <mbudiu@feldera.com>
3 participants
No description provided.