Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-12705] [SQL] push missing attributes for Sort #11153

Closed
wants to merge 5 commits into from

Conversation

davies
Copy link
Contributor

@davies davies commented Feb 10, 2016

The current implementation of ResolveSortReferences can only push one missing attributes into it's child, it failed to analyze TPCDS Q98, because of there are two missing attributes in that (one from Window, another from Aggregate).

@davies
Copy link
Contributor Author

davies commented Feb 10, 2016

cc @marmbrus

@SparkQA
Copy link

SparkQA commented Feb 10, 2016

Test build #51040 has finished for PR 11153 at commit e1919cc.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@davies davies changed the title [SPARK-12705] [SQL] push missing attributes in Sort [SPARK-12705] [SQL] push missing attributes for Sort Feb 10, 2016
@SparkQA
Copy link

SparkQA commented Feb 10, 2016

Test build #2533 has finished for PR 11153 at commit bec639d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@davies
Copy link
Contributor Author

davies commented Feb 11, 2016

cc @gatorsmile

@gatorsmile
Copy link
Member

Sorry, my previous fix does not cover all the scenarios. Will read your fix tomorrow. Thanks!

@SparkQA
Copy link

SparkQA commented Feb 11, 2016

Test build #51085 has finished for PR 11153 at commit c4607dd.

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

case other => (Seq.empty[SortOrder], Seq.empty[Attribute])
val newAggregateExpressions = a.aggregateExpressions ++ missingAttrs
a.copy(aggregateExpressions = newAggregateExpressions)
case u: UnaryNode =>
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we want to cover all the UnaryNode here? This is different from what we discussed. I am fine if you want to add more supports (e.g., crossing the boundary of subquery), but we might need to add more test cases to ensure it does not break anything.

Copy link
Member

Choose a reason for hiding this comment

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

If needed, I can add more test cases after this is merged, since it could be time-consuming. Please feel free to let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will exclude Subquery, feel free to add more tests.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, will do.

@SparkQA
Copy link

SparkQA commented Feb 11, 2016

Test build #51124 has finished for PR 11153 at commit dce3857.

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

@gatorsmile
Copy link
Member

LGTM

@rxin
Copy link
Contributor

rxin commented Feb 12, 2016

cc @cloud-fan for review too

@cloud-fan
Copy link
Contributor

LGTM except one comment

@davies
Copy link
Contributor Author

davies commented Feb 12, 2016

Merging this into master, thanks!

@asfgit asfgit closed this in 5b805df Feb 12, 2016
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