[
https://issues.apache.org/jira/browse/CALCITE-3870?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17690047#comment-17690047
]
Julian Hyde commented on CALCITE-3870:
--------------------------------------
Responding to [~zabetak]'s comments in the PR:
{quote}The tests in [`InterpreterTest`] are only validating results and not
plans. Wouldn't be better to add the SubQueryRemoveRules somewhere in the
fixture (e.g., returnsRows) and always expand as before? Like that we minimize
calls to withExpand and we shift our test coverage towards our recommended
expansion mechanism.
{quote}
Yes, we should apply {{SubQueryRules}}. Just not as part of this task.
{quote}The joinType change from semi to inner is a bit worrisome but in this
case that the right relation has only one element it will not make any
difference I guess.
...
Looks like a beneficial change in terms of performance; the Calc will be
applied on less rows than before (duplicates removed).
{quote}
Yes. The switch from semi-join to an {{Aggregate}} under the right input to an
inner join is fairly neutral in terms of performance. The explicit
{{Aggregate}} allows us to bring back an indicator column (which helps with
tricky cases like {{NOT IN}} where we care not just whether there are any rows,
but whether some/all of them have a null join key).
{quote}There is a big bunch of tests [in RelOptRulesTest] using this fixture. I
suppose you kept it enabled to avoid many plan changes but like that we are
essentially testing more the non-suggested code path.
{quote}
Those tests test the plan before and after firing the rule(s) that are under
test. How the 'before' plan was created is irrelevant to the test.
I agree that we should be more selective. We'd prefer to use the new
{{SqlToRelConverter}} code path as much as possible. Rather than enabling
expansion for all tests, I'll see if I can enable it for a small number of
tests before I merge.
But I won't do anything that causes the 'before' plans to change. That would
create noise in {{RelOptRulesTest}} and possibly regressions.
> Change the default value of SqlToRelConverter.Config.expand from true to false
> ------------------------------------------------------------------------------
>
> Key: CALCITE-3870
> URL: https://issues.apache.org/jira/browse/CALCITE-3870
> Project: Calcite
> Issue Type: Bug
> Components: core
> Reporter: Jiatao Tao
> Assignee: Julian Hyde
> Priority: Minor
> Labels: pull-request-available
> Fix For: 1.34.0
>
> Attachments: image-2020-03-25-14-05-04-011.png
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> Set "SqlToRelConverter.ConfigBuilder#expand" default to false. Now the
> default "expand" in SqlToRelConverter.ConfigBuilder is true but in calcite's
> main process, actually, it is false( {{withExpand(THREAD_EXPAND.get())}})
>
> That leads we need to explicitly set {{withExpand}} to false when we use
> {{SqlToRelConverter}}.
>
> So I think we should change the default to "false" in
> SqlToRelConverter.ConfigBuilder.
>
> !https://mail.google.com/mail/u/0?ui=2&ik=09d5de6bf3&attid=0.1&permmsgid=msg-a:r-1571046416348666034&th=17106774a5349d1d&view=fimg&sz=s0-l75-ft&attbid=ANGjdJ_IjYbiwT8IKlaiAIDzbkq_ehCBktd4DzCYr14Ppst6lBftqMbrrWu8hzGpN2wpCz46a3F1FKvfD_miGHkC-EMx509KPC4FEovxtN2M0WcU1up209hyLtKoZqE&disp=emb&realattid=ii_k8460q950!
> !https://mail.google.com/mail/u/0?ui=2&ik=09d5de6bf3&attid=0.2&permmsgid=msg-a:r-1571046416348666034&th=17106774a5349d1d&view=fimg&sz=s0-l75-ft&attbid=ANGjdJ_K-EyE_x0Vo66h6sHRzpEOxIE5qTzuujqG8WqZVPdprgC3khwcO_8OLLai-8ikrMRC9ksO-xAzqe2HFr-QYM1lzQDLYfSlD5x-eOXbww9CNsb5cgu-979N1lU&disp=emb&realattid=ii_k846zj601!
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
