Skip to content

Conversation

@godfreyhe
Copy link
Contributor

add ProjectMergeRule to merge projections

Copy link
Contributor

@fhueske fhueske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @godfreyhe!
I think the PR is good, but I would remove the explain test. The other plan tests are sufficient to validate that the desired plans are generated.

Thanks, Fabian

}

@Test
def testProjectMerge(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to add a test to ExplainTest.scala. The other plan tests are sufficient to validate that the desired plans are generated.

@@ -0,0 +1,156 @@
== Abstract Syntax Tree ==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file can be removed, IMO.

@fhueske
Copy link
Contributor

fhueske commented Apr 21, 2017

Merging

@asfgit asfgit closed this in 4024aff Apr 21, 2017
StefanRRichter pushed a commit to StefanRRichter/flink that referenced this pull request Apr 24, 2017
PangZhi pushed a commit to PangZhi/flink that referenced this pull request May 1, 2017
@godfreyhe godfreyhe deleted the FLINK-6326 branch May 2, 2017 12:25
StefanRRichter pushed a commit to StefanRRichter/flink that referenced this pull request May 5, 2017
fanyon pushed a commit to fanyon/flink that referenced this pull request May 11, 2017
hequn8128 pushed a commit to hequn8128/flink that referenced this pull request Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants