[ 
https://issues.apache.org/jira/browse/CALCITE-5727?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17740710#comment-17740710
 ] 

Tanner Clary commented on CALCITE-5727:
---------------------------------------

I messed up the commit chain, the more accurate commit link is 
[here|https://github.com/apache/calcite/commit/b94917883cc55ba5ed034e2f9a8b55e961036b63],
 sorry for any inconvenience.

> RelOptFixture#checkUnchanged should assert planAfter is not present
> -------------------------------------------------------------------
>
>                 Key: CALCITE-5727
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5727
>             Project: Calcite
>          Issue Type: Improvement
>          Components: tests
>    Affects Versions: 1.34.0
>            Reporter: Benchao Li
>            Assignee: Benchao Li
>            Priority: Minor
>              Labels: pull-request-available
>             Fix For: 1.35.0
>
>
> This is discovered while I'm review another 
> [PR|https://github.com/apache/calcite/pull/3193#discussion_r1204998877]. And 
> there are already some tests that using {{{}checkUnchanged{}}}, but the xml 
> file still contains {{planAfter}} section, such as 
> {{{}RelOptRulesTest#testNoReduceAverage{}}}.
> IMO, there is no need to keep {{planAfter}} for {{{}checkUnchanged{}}}, and 
> it does more harm than good to keep it. Even we can make {{planAfter}} the 
> same with {{{}planBefore{}}}, it will bring burden for reading (because we'll 
> identify the difference of planAfter and planBefore). And it will be more 
> harmful when {{planAfter}} is different from {{{}planBefore{}}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to