Skip to content

Conversation

@hvanhovell
Copy link
Contributor

This PR is a follow-up for PR #9819. It adds documentation for the window functions and a couple of NULL tests.

The documentation was largely based on the documentation in (the source of) Hive and Presto:

I am not sure if we need to add the licenses of these two projects to the licenses directory. They are both under the ASL. @srowen any thoughts?

cc @yhuai

@hvanhovell
Copy link
Contributor Author

ok to test

@SparkQA
Copy link

SparkQA commented Dec 20, 2015

Test build #48080 has finished for PR 10402 at commit 2b806bb.

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

@yhuai
Copy link
Contributor

yhuai commented Dec 21, 2015

@hvanhovell I just created https://issues.apache.org/jira/browse/SPARK-12455 for adding doc. Can you also put SPARK-12455 in the jira title?

@hvanhovell hvanhovell changed the title [SPARK-8641][SQL] Native Spark Window functions - Follow-up (docs & tests) [SPARK-8641][SPARK-12455][SQL] Native Spark Window functions - Follow-up (docs & tests) Dec 21, 2015
@hvanhovell
Copy link
Contributor Author

Done.

@gatorsmile
Copy link
Member

I guess you also need to put extended like the example Upper given by @yhuai ?

@hvanhovell
Copy link
Contributor Author

@gatorsmile extended is typically used to give an example of how the functions is used. See: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionDescription.java#L32-L33

I thought of adding this, but it is quite hard to create clear and concise examples for these functions. I am open to suggestions...

@gatorsmile
Copy link
Member

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Add some comments to explain the implementation of NTile?

@yhuai
Copy link
Contributor

yhuai commented Dec 21, 2015

@hvanhovell Regarding the doc, we can acknowledge them (Hive and Presto) in the scala doc.

@SparkQA
Copy link

SparkQA commented Dec 24, 2015

Test build #48294 has finished for PR 10402 at commit cf58954.

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

@SparkQA
Copy link

SparkQA commented Dec 28, 2015

Test build #48362 has finished for PR 10402 at commit 767305a.

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

@SparkQA
Copy link

SparkQA commented Dec 28, 2015

Test build #48363 has finished for PR 10402 at commit 13f9c95.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a clarification question. For our current code, default will never be null, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently always pass a literal(null) if no default is specified. However a user can pass a null value if (s)he wants to; there is no check to prevent this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW: This is not a problem, all code should be able to deal with a null default expression

@yhuai
Copy link
Contributor

yhuai commented Dec 31, 2015

LGTM. Merging to master.

@asfgit asfgit closed this in f76ee10 Dec 31, 2015
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.

4 participants