[
https://issues.apache.org/jira/browse/CALCITE-7016?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17951610#comment-17951610
]
Zhen Chen edited comment on CALCITE-7016 at 5/15/25 6:13 AM:
-------------------------------------------------------------
[~julianhyde] Thanks for your reply. In RelOptRulesTest, Calcite currently
supports two approaches: sql() and relFn(). When using the sql() method, it
works as expected - the SQL is properly displayed in the XML since it has
logical correlations with both planBefore and planAfter.However, when using
relFn(), the current implementation doesn't validate SQL resources added in the
XML - arbitrary SQL can be written without any checks, which violates
expectations. If I understand correctly, the plan constructed by relFn() should
match planBefore. Therefore, the proposed fix is to: Throw an error if relFn()
is used while SQL is present in the XML. Like this:
same as current testFilterSortTranspose
{code:java}
@Test void testUsingRelFnWithRedundantSql() {
final Function<RelBuilder, RelNode> relFn = b -> b
.scan("EMP")
.project(b.field(0))
.sort(b.field(0))
.filter(b.lessThan(b.field(0), b.literal(10)))
.build();
relFn(relFn).withRule(CoreRules.FILTER_SORT_TRANSPOSE).check();
} {code}
correct XML
{code:java}
<TestCase name="testUsingRelFnWithRedundantSql">
<Resource name="planBefore">
<![CDATA[
LogicalFilter(condition=[<($0, 10)])
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
<Resource name="planAfter">
<![CDATA[
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalFilter(condition=[<($0, 10)])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
</TestCase> {code}
add sql resource
{code:java}
<TestCase name="testUsingRelFnWithRedundantSql">
<Resource name="sql">
<![CDATA[xxxxxxxx
]]>
</Resource>
<Resource name="planBefore">
<![CDATA[
LogicalFilter(condition=[<($0, 10)])
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
<Resource name="planAfter">
<![CDATA[
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalFilter(condition=[<($0, 10)])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
</TestCase> {code}
can throw error
{code:java}
relFn test case should not contain sql resource in XML file
java.lang.AssertionError: relFn test case should not contain sql resource in
XML file
at
org.apache.calcite.test.RelSupplier$FnRelSupplier.apply(RelSupplier.java:133)
at org.apache.calcite.test.RelOptFixture.toRel(RelOptFixture.java:309)
at
org.apache.calcite.test.RelOptFixture.checkPlanning(RelOptFixture.java:345)
at org.apache.calcite.test.RelOptFixture.check(RelOptFixture.java:334)
at org.apache.calcite.test.RelOptFixture.check(RelOptFixture.java:318)
at
org.apache.calcite.test.RelOptRulesTest.testUsingRelFnWithRedundantSql(RelOptRulesTest.java:10731)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569) {code}
Would this solution be acceptable?
was (Author: jensen):
[~julianhyde] Thanks for your reply. In RelOptRulesTest, Calcite currently
supports two approaches: sql() and relFn(). When using the sql() method, it
works as expected - the SQL is properly displayed in the XML since it has
logical correlations with both planBefore and planAfter.However, when using
relFn(), the current implementation doesn't validate SQL resources added in the
XML - arbitrary SQL can be written without any checks, which violates
expectations. If I understand correctly, the plan constructed by relFn() should
match planBefore. Therefore, the proposed fix is to: Throw an error if relFn()
is used while SQL is present in the XML. Like this:
// same as current testFilterSortTranspose
@Test void testUsingRelFnWithRedundantSql() {
final Function<RelBuilder, RelNode> relFn = b -> b
.scan("EMP")
.project(b.field(0))
.sort(b.field(0))
.filter(b.lessThan(b.field(0), b.literal(10)))
.build();
relFn(relFn).withRule(CoreRules.FILTER_SORT_TRANSPOSE).check();
}
// correct XML
<TestCase name="testUsingRelFnWithRedundantSql">
<Resource name="planBefore">
<![CDATA[
LogicalFilter(condition=[<($0, 10)])
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
<Resource name="planAfter">
<![CDATA[
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalFilter(condition=[<($0, 10)])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
</TestCase>
// add sql resource
<TestCase name="testUsingRelFnWithRedundantSql">
<Resource name="sql">
<![CDATA[xxxxxxxx
]]>
</Resource>
<Resource name="planBefore">
<![CDATA[
LogicalFilter(condition=[<($0, 10)])
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
<Resource name="planAfter">
<![CDATA[
LogicalSort(sort0=[$0], dir0=[ASC])
LogicalFilter(condition=[<($0, 10)])
LogicalProject(EMPNO=[$0])
LogicalTableScan(table=[[scott, EMP]])
]]>
</Resource>
</TestCase>
// can throw error
relFn test case should not contain sql resource in XML file
java.lang.AssertionError: relFn test case should not contain sql resource in
XML file
at
org.apache.calcite.test.RelSupplier$FnRelSupplier.apply(RelSupplier.java:133)
at org.apache.calcite.test.RelOptFixture.toRel(RelOptFixture.java:309)
at
org.apache.calcite.test.RelOptFixture.checkPlanning(RelOptFixture.java:345)
at org.apache.calcite.test.RelOptFixture.check(RelOptFixture.java:334)
at org.apache.calcite.test.RelOptFixture.check(RelOptFixture.java:318)
at
org.apache.calcite.test.RelOptRulesTest.testUsingRelFnWithRedundantSql(RelOptRulesTest.java:10731)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
Would this solution be acceptable?
> Sql resources should not be included in XML when using relFn in
> RelOptRulesTest testing
> ---------------------------------------------------------------------------------------
>
> Key: CALCITE-7016
> URL: https://issues.apache.org/jira/browse/CALCITE-7016
> Project: Calcite
> Issue Type: Improvement
> Reporter: Zhen Chen
> Assignee: Zhen Chen
> Priority: Minor
>
> Use relFn to construct a plan for rule validation, and SQL resources should
> not be included in the result file(RelOptRulesTest.xml). If SQL can be used
> to generate plans, it is not recommended to use relFn for construction.
> Otherwise, it may lead to incorrect mapping between sql and planBefore.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
