Skip to content

Conversation

@petermaxlee
Copy link
Contributor

@petermaxlee petermaxlee commented Sep 17, 2016

What changes were proposed in this pull request?

This pull request adds Scala/Java DataFrame API for null ordering (NULLS FIRST | LAST).

Also did some minor clean up for related code (e.g. incorrect indentation), and renamed "orderby-nulls-ordering.sql" to be consistent with existing test files.

How was this patch tested?

Added a new test case in DataFrameSuite.

@petermaxlee
Copy link
Contributor Author

petermaxlee commented Sep 17, 2016

This is based on #15107 but cleaned up a little bit.

cc @hvanhovell and @xwu0226

}

test("sorting with null ordering") {
val data = Seq[java.lang.Integer](2, 1, null).toDF("key")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a much simpler test case that the one added in #15107.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the cleanup and refining the test case! For the test case with 2 column data, I was thinking to create a bit more complex ordering to utilize the prefix sort and regular GenerateOrdering in one query. But it is not necessary since the major PR for SQL interface has include such scenario.

@SparkQA
Copy link

SparkQA commented Sep 17, 2016

Test build #65517 has finished for PR 15123 at commit 02f5edf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SortOrder(child: Expression, direction: SortDirection, nullOrdering: NullOrdering)

@petermaxlee
Copy link
Contributor Author

cc @hvanhovell

@hvanhovell
Copy link
Contributor

LGTM - merging to master. Thanks.

@asfgit asfgit closed this in de333d1 Sep 25, 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.

4 participants