Skip to content

[SPARK-9286] [SQL] Methods in Unevaluable should be final and AlgebraicAggregate should extend Unevaluable. #7627

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[SPARK-9286] [SQL] Methods in Unevaluable should be final and AlgebraicAggregate should extend Unevaluable. #7627

wants to merge 3 commits into from

Conversation

@JoshRosen
Copy link
Contributor

@JoshRosen JoshRosen commented Jul 23, 2015

This patch marks the Unevaluable.eval() and UnevaluablegenCode() methods as final and fixes two cases where they were overridden. It also updates AggregateFunction2 to extend Unevaluable.

@JoshRosen JoshRosen mentioned this pull request Jul 23, 2015
Closed
@JoshRosen JoshRosen changed the title [SPARK-9286] [SQL] Methods in Unevaluable should be final [SPARK-9286] [SQL] Methods in Unevaluable should be final and AggregateFunction2 should extend Unevalable. Jul 23, 2015
@JoshRosen JoshRosen changed the title [SPARK-9286] [SQL] Methods in Unevaluable should be final and AggregateFunction2 should extend Unevalable. [SPARK-9286] [SQL] Methods in Unevaluable should be final and AggregateFunction2 should extend Unevaluable. Jul 23, 2015
@JoshRosen
Copy link
Contributor Author

JoshRosen commented Jul 23, 2015

Hold on one sec... going to update this so that AggregateFunction2 also extends Unevaluable.

JoshRosen
JoshRosen reviewed Jul 23, 2015
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/interfaces.scala
Copy link
Contributor Author

@JoshRosen JoshRosen Jul 23, 2015

Choose a reason for hiding this comment

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

Did we need this?

Copy link
Contributor

@yhuai yhuai Jul 23, 2015

Choose a reason for hiding this comment

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

I think we do not need it.

@JoshRosen
Copy link
Contributor Author

JoshRosen commented Jul 23, 2015

/cc @yhuai for review.

@yhuai
Copy link
Contributor

yhuai commented Jul 23, 2015

AggregateFunction2 should extend Unevaluable. => AlgebraicAggregate should extend Unevaluable.

@JoshRosen JoshRosen changed the title [SPARK-9286] [SQL] Methods in Unevaluable should be final and AggregateFunction2 should extend Unevaluable. [SPARK-9286] [SQL] Methods in Unevaluable should be final and AlgebraicAggregate should extend Unevaluable. Jul 23, 2015
@JoshRosen
Copy link
Contributor Author

JoshRosen commented Jul 23, 2015

Good catch; updated.

@SparkQA
Copy link

SparkQA commented Jul 23, 2015

Test build #38267 has finished for PR 7627 at commit 65329c2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class AggregateFunction1 extends LeafExpression with Serializable

@yhuai
Copy link
Contributor

yhuai commented Jul 23, 2015

LGTM. Merging it to master.

@asfgit asfgit closed this in b2f3aca Jul 23, 2015
@SparkQA
Copy link

SparkQA commented Jul 24, 2015

Test build #38275 has finished for PR 7627 at commit 8d9ed22.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class AlgebraicAggregate extends AggregateFunction2 with Serializable with Unevaluable
    • abstract class AggregateFunction1 extends LeafExpression with Serializable

@JoshRosen JoshRosen deleted the unevaluable-fix branch August 29, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JoshRosen @yhuai @SparkQA