[SPARK-12789] [SQL] Support Order By Ordinal in SQL #11815

gatorsmile · 2016-03-18T06:42:47Z

What changes were proposed in this pull request?

This PR is to support order by position in SQL, e.g.

select c1, c2, c3 from tbl order by 1 desc, 3

should be equivalent to

select c1, c2, c3 from tbl order by c1 desc, c3 asc

This is controlled by config option spark.sql.orderByOrdinal.

When true, the ordinal numbers are treated as the position in the select list.
When false, the ordinal number in order/sort By clause are ignored.
Only convert integer literals (not foldable expressions). If found foldable expressions, ignore them
This also works with select *.

Question: Do we still need sort by columns that contain zero reference? In this case, it will have no impact on the sorting results. IMO, we should not allow users do it. @rxin @cloud-fan @marmbrus @yhuai @hvanhovell
-- Update: In these cases, they are ignored in this case.

Note: This PR is taken from #10731. When merging this PR, please give the credit to @zhichao-li

Also cc all the people who are involved in the previous discussion: @adrian-wang @chenghao-intel @tejasapatil

How was this patch tested?

Added a few test cases for both positive and negative test cases.

rxin · 2016-03-18T06:43:57Z

Thanks - the description is great!

If the order by list is empty (all or constant foldable), then I'd say we should get rid of the order by. That's what postgres does too.

Update: no it has nothing to do with postgres. But I still think when that flag is off, we should accept ints and they are just noops.

rxin · 2016-03-18T06:45:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala

+object IntegerIndex {
+  def unapply(a: Any): Option[Int] = a match {
+    case Literal(a: Int, IntegerType) => Some(a)
+    case UnaryMinus(IntegerLiteral(v)) => Some(-v)


i still don't get it - why are we matching unary minus ?

As shown in the following link, our parser converts -3 to UnaryMinus(Literal(3)). Thus, we have to use this to get negative values.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/CatalystQl.scala

Line 641 in 637a78f

case Token("-", child :: Nil) => UnaryMinus(nodeToExpr(child))

But why are we getting negative values? Ordinals can only be positive.

When users input negative values, we just detect and then issue an exception message, like

https://github.com/gatorsmile/spark/blob/orderByPosition/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala#L644-L646

ah ic so this is used to detect errors. i'd add some comment here explaining why we are having this.

sure, will do. Thanks!

SparkQA · 2016-03-18T08:07:30Z

Test build #53516 has finished for PR 11815 at commit e0dce39.

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

gatorsmile · 2016-03-18T16:04:28Z

Will submit a separate PR for cleaning/eliminating no-op SortOrder in Optimizer. Thanks!

SparkQA · 2016-03-18T17:32:25Z

Test build #53542 has finished for PR 11815 at commit 256814b.

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

rxin · 2016-03-18T18:33:58Z

cc @cloud-fan for review

cloud-fan · 2016-03-19T05:10:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+      // Replace the index with the related attribute for ORDER BY
+      // which is a 1-base position of the projection list.
+      case s @ Sort(orders, global, child)
+          if conf.orderByOrdinal && child.resolved &&


nit: we can add a case above: case s: Sort if !s.child.resolved => s, then we don't need to check child.resolved in following cases.

Sure, will do. Thanks!

cloud-fan · 2016-03-19T05:17:28Z

LGTM except 2 minor comments

SparkQA · 2016-03-19T07:22:59Z

Test build #53604 has finished for PR 11815 at commit b04529b.

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

adrian-wang · 2016-03-19T07:35:27Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -435,6 +435,11 @@ object SQLConf {
    defaultValue = Some(true),
    doc = "When false, we will treat bucketed table as normal table")

+  val ORDER_BY_ORDINAL = booleanConf("spark.sql.orderByOrdinal",


There's a similar configuration value in hive since 0.11 and later, which is hive.groupby.orderby.position.alias(default as false).
See: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+SortBy#LanguageManualSortBy-SyntaxofOrderBy

I think we also need to read from ConfVar for this config.

I don't mind just diverging from Hive over time and not support any of the config options.

Sounds good to me.

@adrian-wang Thank you for your input! Glad to learn it.

cloud-fan · 2016-03-21T06:07:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+              SortOrder(child.output(index - 1), direction)
+            } else {
+              throw new UnresolvedException(s,
+                "Order/sort By position: $index does not exist " +


seems you miss a s at head? Should be s"... $index..."

True. Let me correct it. Thanks!

SparkQA · 2016-03-21T06:46:59Z

Test build #53656 has finished for PR 11815 at commit d52e1c7.

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

cloud-fan · 2016-03-21T07:48:07Z

LGTM, pending tests.

SparkQA · 2016-03-21T09:03:39Z

Test build #53662 has finished for PR 11815 at commit 46e3f69.

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

#### What changes were proposed in this pull request? This PR is to support order by position in SQL, e.g. ```SQL select c1, c2, c3 from tbl order by 1 desc, 3 ``` should be equivalent to ```SQL select c1, c2, c3 from tbl order by c1 desc, c3 asc ``` This is controlled by config option `spark.sql.orderByOrdinal`. - When true, the ordinal numbers are treated as the position in the select list. - When false, the ordinal number in order/sort By clause are ignored. - Only convert integer literals (not foldable expressions). If found foldable expressions, ignore them - This also works with select *. **Question**: Do we still need sort by columns that contain zero reference? In this case, it will have no impact on the sorting results. IMO, we should not allow users do it. rxin cloud-fan marmbrus yhuai hvanhovell -- Update: In these cases, they are ignored in this case. **Note**: This PR is taken from apache#10731. When merging this PR, please give the credit to zhichao-li Also cc all the people who are involved in the previous discussion: adrian-wang chenghao-intel tejasapatil #### How was this patch tested? Added a few test cases for both positive and negative test cases. Author: gatorsmile <[email protected]> Closes apache#11815 from gatorsmile/orderByPosition.

gatorsmile added 4 commits March 17, 2016 23:15

Order by ordinal number.

aee1023

Merge remote-tracking branch 'upstream/master' into orderByPosition

3342798

style fix.

240aeb3

improve the test cases.

e0dce39

rxin reviewed Mar 18, 2016
View reviewed changes

address comments.

256814b

cloud-fan reviewed Mar 19, 2016
View reviewed changes

address comments.

b04529b

adrian-wang reviewed Mar 19, 2016
View reviewed changes

address comments.

d52e1c7

cloud-fan reviewed Mar 21, 2016
View reviewed changes

gatorsmile added 2 commits March 20, 2016 23:53

Merge remote-tracking branch 'upstream/master' into orderByPosition

4d44b75

address comments.

46e3f69

asfgit closed this in 2c5b18f Mar 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-12789] [SQL] Support Order By Ordinal in SQL #11815

[SPARK-12789] [SQL] Support Order By Ordinal in SQL #11815

gatorsmile commented Mar 18, 2016

rxin commented Mar 18, 2016

rxin Mar 18, 2016

gatorsmile Mar 18, 2016

rxin Mar 18, 2016

gatorsmile Mar 18, 2016

rxin Mar 18, 2016

gatorsmile Mar 18, 2016

SparkQA commented Mar 18, 2016

gatorsmile commented Mar 18, 2016

SparkQA commented Mar 18, 2016

rxin commented Mar 18, 2016

cloud-fan Mar 19, 2016

gatorsmile Mar 19, 2016

cloud-fan commented Mar 19, 2016

SparkQA commented Mar 19, 2016

adrian-wang Mar 19, 2016

rxin Mar 19, 2016

adrian-wang Mar 19, 2016

gatorsmile Mar 19, 2016

cloud-fan Mar 21, 2016

gatorsmile Mar 21, 2016

SparkQA commented Mar 21, 2016

cloud-fan commented Mar 21, 2016

SparkQA commented Mar 21, 2016

[SPARK-12789] [SQL] Support Order By Ordinal in SQL #11815

[SPARK-12789] [SQL] Support Order By Ordinal in SQL #11815

Conversation

gatorsmile commented Mar 18, 2016

What changes were proposed in this pull request?

How was this patch tested?

rxin commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 18, 2016

gatorsmile commented Mar 18, 2016

SparkQA commented Mar 18, 2016

rxin commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 19, 2016

SparkQA commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 21, 2016

cloud-fan commented Mar 21, 2016

SparkQA commented Mar 21, 2016