[CALCITE-3346] Enable some ignored tests in RelOptRuleTests by yanlin-Lynn · Pull Request #1453 · apache/calcite

yanlin-Lynn · 2019-09-14T06:59:14Z

This PR tries to re-enable some ignored tests in RelOptRuleTests.

amaliujia · 2019-09-16T23:30:06Z

To re-enable ignored tests is definitely a good idea. I will spend some time to check how those ignored tests in PR work(I see some changes on tests and not sure if they have changed intentions of original tests).

Meanwhile, I am seeing other style fixes in this PR. Maybe it's better to send a hot fix PR to fix those styles than mixing with CALCITE-3346?

yanlin-Lynn · 2019-09-17T02:42:57Z

To re-enable ignored tests is definitely a good idea. I will spend some time to check how those ignored tests in PR work(I see some changes on tests and not sure if they have changed intentions of original tests).

Meanwhile, I am seeing other style fixes in this PR. Maybe it's better to send a hot fix PR to fix those styles than mixing with CALCITE-3346?

Thanks @amaliujia . It will be helpful with someone to check again.
And I remove the style related fixs.

amaliujia

@danny0405

Do you have recommendation on who could be better person to review this PR?

amaliujia · 2019-09-19T19:13:53Z

+        </Resource>
+        <Resource name="planBefore">
+            <![CDATA[
+LogicalTableModify(table=[[CATALOG, SALES, DEPT]], operation=[INSERT], flattened=[false])


I can see what's being tested here, but not sure if it is the original intention of that ignored test as you have added a SELECT there. I guess the original test is used to test if NULL works when applying ValuesReduceRule, and the new test does not include it.

Will need a committer who is more familiar with optimization to take a look then.

I update the PR, add the NULL case.

amaliujia · 2019-09-19T19:19:01Z

+        <Resource name="planAfter">
+            <![CDATA[
+LogicalCalc(expr#0..10=[{inputs}], expr#11=[=($t7, $t9)], expr#12=['Propane':VARCHAR(10)], expr#13=[=($t10, $t12)], expr#14=[AND($t11, $t13)], DNAME=[$t10], ENAME=[$t1], $condition=[$t14])
+  LogicalJoin(condition=[true], joinType=[inner])


Not sure when this optimization could be useful: it change a inner equi-join to cross join and move the filter to Calc. So it's doing a filter moving up.

Also need a better person to check this one.

Yes, that's the purpose of JoinExtractFilterRule. It isn't widely used, but it is a valid transformation. Don't worry too much about whether these are "optimizations"; just ask whether they are semantically valid rewrites.

There are quite a few rules that are useful in matching materialized views that do not seem to make the plan cheaper.

danny0405 · 2019-09-20T01:29:57Z

@danny0405

Do you have recommendation on who could be better person to review this PR?

I can help to review, but maybe the weekend, i'm so busy this week days.

danny0405

+1

julianhyde · 2019-09-24T21:52:38Z

The transitiveInference method was always a bit weird, and different from other tests. But at least it made things concise. After these changes, adding a sql parameter, it is no longer concise. Can we get rid of it, so that the tests look like other tests?

Use your judgment to create tests that are readable, clear, maintainable, without too much redundancy.

yanlin-Lynn · 2019-09-26T02:30:20Z

@danny0405 , what do you think?
Should I change transitiveInference back to the old way, without sql parameter, as @julianhyde suggested?

danny0405 · 2019-09-26T04:11:05Z

@danny0405 , what do you think?
Should I change transitiveInference back to the old way, without sql parameter, as @julianhyde suggested?

If i understand correctly, Julian means to do more refactoring for the method so it looks more concise ? Or we should revert the change @julianhyde ?

…Yanlin)

yanlin-Lynn · 2019-09-29T05:34:43Z

how about this time, I make a refactor.

danny0405 · 2019-09-29T05:48:42Z

how about this time, I make a refactor.

Thanks for the effort, now the tests looks the same as others, which is more intuitive, i'm +1 for this. @julianhyde , do you have any objections ?

julianhyde · 2019-09-29T16:12:16Z

+1

Looks great after refactor.

yanlin-Lynn force-pushed the tests branch 2 times, most recently from b0d30ee to 9147394 Compare September 14, 2019 09:21

julianhyde force-pushed the master branch from 2c19098 to f3758a6 Compare September 16, 2019 17:44

yanlin-Lynn force-pushed the tests branch from 9147394 to bd66e85 Compare September 17, 2019 02:37

amaliujia reviewed Sep 19, 2019

View reviewed changes

yanlin-Lynn force-pushed the tests branch from bd66e85 to a4a14f8 Compare September 20, 2019 02:53

danny0405 force-pushed the master branch from df3dc81 to 883e191 Compare September 20, 2019 04:17

danny0405 reviewed Sep 21, 2019

View reviewed changes

Comment thread core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java

yanlin-Lynn force-pushed the tests branch 2 times, most recently from 8cc1568 to 46c0bb2 Compare September 23, 2019 08:17

danny0405 approved these changes Sep 23, 2019

View reviewed changes

hsyuan reviewed Sep 24, 2019

View reviewed changes

Comment thread core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml Outdated

yanlin-Lynn force-pushed the tests branch from 46c0bb2 to 64615d6 Compare September 24, 2019 08:04

danny0405 reviewed Sep 24, 2019

View reviewed changes

Comment thread core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java Outdated

yanlin-Lynn force-pushed the tests branch 2 times, most recently from b840a1b to f53952e Compare September 24, 2019 09:03

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 24, 2019

yanlin-Lynn force-pushed the tests branch from f53952e to 56711fc Compare September 26, 2019 02:41

[CALCITE-3346] Re-enable some ignored tests in RelOptRuleTests (Wang …

7b1d032

…Yanlin)

yanlin-Lynn force-pushed the tests branch from a1bc414 to 7b1d032 Compare September 29, 2019 03:49

hsyuan merged commit 87c5bd2 into apache:master Oct 3, 2019

yanlin-Lynn deleted the tests branch October 3, 2019 23:45

Uh oh!

Conversation

yanlin-Lynn commented Sep 14, 2019

Uh oh!

amaliujia commented Sep 16, 2019

Uh oh!

yanlin-Lynn commented Sep 17, 2019

Uh oh!

amaliujia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaliujia Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

yanlin-Lynn Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

amaliujia Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianhyde Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Sep 20, 2019

Uh oh!

Uh oh!

danny0405 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

julianhyde commented Sep 24, 2019

Uh oh!

yanlin-Lynn commented Sep 26, 2019

Uh oh!

danny0405 commented Sep 26, 2019

Uh oh!

yanlin-Lynn commented Sep 29, 2019

Uh oh!

danny0405 commented Sep 29, 2019

Uh oh!

julianhyde commented Sep 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amaliujia Sep 19, 2019 •

edited

Loading