Skip to content

[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters #17876

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
cloud-fan wants to merge 1 commit into from
Closed

[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters #17876

cloud-fan wants to merge 1 commit into from

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented May 5, 2017

What changes were proposed in this pull request?

RuntimeReplaceable always has a constructor with the expression to replace with, and this constructor should not be the function builder.

How was this patch tested?

new regression test

@cloud-fan
Copy link
Contributor Author

cloud-fan commented May 5, 2017

cc @gatorsmile @marmbrus

@SparkQA
Copy link

SparkQA commented May 5, 2017

Test build #76501 has finished for PR 17876 at commit 431a8cd.

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

@SparkQA
Copy link

SparkQA commented May 8, 2017

Test build #76570 has finished for PR 17876 at commit fcbeeb9.

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

@SparkQA
Copy link

SparkQA commented May 8, 2017

Test build #76587 has finished for PR 17876 at commit 0021ec3.

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

@cloud-fan
Copy link
Contributor Author

cloud-fan commented May 9, 2017

retest this please

@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76599 has finished for PR 17876 at commit 0021ec3.

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

@cloud-fan
Copy link
Contributor Author

cloud-fan commented May 9, 2017

retest this please

@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76609 has finished for PR 17876 at commit 0021ec3.

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

@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76616 has finished for PR 17876 at commit 601e988.

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

@gatorsmile
Copy link
Member

gatorsmile commented May 9, 2017

retest this please

@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76654 has finished for PR 17876 at commit 601e988.

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

gatorsmile
gatorsmile reviewed May 11, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
val maxNumArgs = all.map(_.getParameterCount).max
all.filterNot(_.getParameterCount == maxNumArgs)
} else {
tag.runtimeClass.getConstructors
Copy link
Member

@gatorsmile gatorsmile May 11, 2017

Choose a reason for hiding this comment

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

What is the reason why we originally called getDeclaredConstructor instead of getConstructor?

Copy link
Contributor Author

@cloud-fan cloud-fan May 11, 2017

Choose a reason for hiding this comment

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

getDeclaredConstructor will find a specific constructor matching the given parameter types. Now I have some special logic about choosing the constructor, so I call getConstructors to get all the constructors.

@gatorsmile
Copy link
Member

gatorsmile commented May 11, 2017

LGTM

asfgit pushed a commit that referenced this pull request May 11, 2017
@cloud-fan @gatorsmile
[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra…
dd9e3b2 
… parameters

## What changes were proposed in this pull request?

`RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder.

## How was this patch tested?

new regression test

Author: Wenchen Fan <[email protected]>

Closes #17876 from cloud-fan/minor.

(cherry picked from commit b4c99f4)
Signed-off-by: Xiao Li <[email protected]>
@gatorsmile
Copy link
Member

gatorsmile commented May 11, 2017 •
edited
Loading

Thanks! Merging to master/2.2

@asfgit asfgit closed this in b4c99f4 May 11, 2017
lycplus pushed a commit to lycplus/spark that referenced this pull request May 24, 2017
@cloud-fan @lycplus
[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra…
501fc78 
… parameters

## What changes were proposed in this pull request?

`RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder.

## How was this patch tested?

new regression test

Author: Wenchen Fan <[email protected]>

Closes apache#17876 from cloud-fan/minor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

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

@cloud-fan @SparkQA @gatorsmile