You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@calcite.apache.org by "Zhen Chen (Jira)" <ji...@apache.org> on 2025/05/15 06:14:00 UTC
[jira] [Comment Edited] (CALCITE-7016) Sql resources should not be included in XML when using relFn in RelOptRulesTest testing
[ 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)