Skip to content

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Jul 31, 2018

What changes were proposed in this pull request?

This is a followup of #21822

Similar to TreeNode, AnalysisHelper should also provide 3 versions of transformations: resolveOperatorsUp, resolveOperatorsDown and resolveOperators.

This PR adds the missing resolveOperatorsUp, and also fixes some code style which is missed in #21822

How was this patch tested?

existing tests.

@holdensmagicalunicorn
Copy link

@cloud-fan, thanks! I am a bot who has found some folks who might be able to help with the review:@gatorsmile, @yhuai and @rxin

@rxin
Copy link
Contributor

rxin commented Jul 31, 2018

Do we really need this? It's almost always the case for resolution that you'd want to do bottom up, so I thought Michael's original design to just call it resolveOperators make a lot of sense.

@cloud-fan
Copy link
Contributor Author

@rxin there are different patterns

  1. only match a node if its children are resolved. This is the most common pattern and we should transform up.
  2. match a specific node without checking if children are resolved. This pattern doesn't care about transform direction.
  3. match a group of nodes, like case Filter(cond, Aggregate(...)). This pattern needs transform down.

It's a little confusing to only have resolveOperators and resolveOperatorsDown, especially we change some rules from transform to resolveOperatorsDown. It took me a while to think about why we specify the down direction for these rules, then I realized TreeNode.transform calls transformDown and we want to keep the previous transform direction.

@SparkQA
Copy link

SparkQA commented Jul 31, 2018

Test build #93833 has finished for PR 21932 at commit 1f24468.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

It sounds reasonable to me. cc @rxin

@SparkQA
Copy link

SparkQA commented Aug 7, 2018

Test build #94348 has finished for PR 21932 at commit 9d12a9e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • s\"its class is $

@cloud-fan
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Aug 7, 2018

Test build #94351 has finished for PR 21932 at commit 9d12a9e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • s\"its class is $

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in 1a29fec Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants