[SPARK-18134][SQL] Orderable MapType #19330

jinxing64 · 2017-09-23T06:51:05Z

What changes were proposed in this pull request?

We can make MapType orderable, and thus usable in aggregates and joins.

How was this patch tested?

Unit tests.

jinxing64 · 2017-09-23T06:51:53Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+    case map: MapType =>
+      val compareFunc = freshName("compareMap")
+      val compare = this.addReferenceObj("compare", map.interpretedOrdering,
+        classOf[Ordering[MapData]].getCanonicalName)


TODO: we should code generate this.

jinxing64 · 2017-09-23T06:54:24Z

It seems #15970 is not being worked.
I resolved conflicts and add some tests in this pr.

SparkQA · 2017-09-23T07:04:44Z

Test build #82106 has finished for PR 19330 at commit 2e2b98d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2017-09-23T07:41:58Z

@jinxing64 thanks for taking over. I have glanced over the PR, and I miss the explicit sorting of maps. We can't assume that maps are sorted (or even have unique keys) out of the box, for example the following example should evaluate to true but it won't in your PR: map(1, 2, 3, 4) = map(3, 4, 1, 2).

SparkQA · 2017-09-23T09:55:06Z

Test build #82107 has finished for PR 19330 at commit bd50495.

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

tejasapatil · 2017-09-23T17:39:08Z

@hvanhovell : based on your comment over the jira, it seemed that the approach to be used is yet to be finalised. Are we moving ahead with this approach ?

jinxing64 · 2017-09-24T04:43:10Z

@hvanhovell @tejasapatil
Thanks a lot for comment.
I got your point. I will refine soon.

SparkQA · 2017-09-26T07:04:43Z

Test build #82174 has finished for PR 19330 at commit 0385487.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2017-09-26T07:04:44Z

Test build #82173 has finished for PR 19330 at commit 33e532b.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

jinxing64 · 2017-09-26T11:30:41Z

Jenkins, retest this plesase.

SparkQA · 2017-09-26T16:13:35Z

Test build #82196 has finished for PR 19330 at commit 5279338.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

jinxing64 · 2017-09-27T03:02:44Z

It seems the failed SparkR unit tests is not related.
In current change, I added trait OrderSpecified, expressions(BinaryComparison, Max, Min, SortArray, SortOrder) using ordering of data type extends OrderSpecified. During analyzing, expressions with map type output will be wrapped in OrderMaps , thus changed to be an ordered map.
Expressions with map type output in Aggregate, Distinct and SetOperation will also be wrapped in OrderMaps.

kiszk · 2017-09-27T09:22:00Z

Jenkins, retest this please

SparkQA · 2017-09-27T12:09:59Z

Test build #82232 has finished for PR 19330 at commit 5279338.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

jinxing64 · 2017-09-27T13:37:23Z

Conflicts resolved.

SparkQA · 2017-09-27T16:28:22Z

Test build #82242 has finished for PR 19330 at commit e071892.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

kiszk · 2017-09-27T18:52:50Z

@jinxing64 While I am not familiar with R, may we have to add one more value for ordered at this line?

kiszk · 2017-09-27T19:25:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/MapType.scala

+    keyType: DataType,
+    valueType: DataType,
+    valueContainsNull: Boolean,
+    ordered: Boolean = false) extends DataType {


Do we need to take care of ordered in buildFormattedString and jsonValue?

SparkQA · 2017-09-28T04:48:05Z

Test build #82264 has finished for PR 19330 at commit 0b2b52c.

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

SparkQA · 2017-09-28T10:17:39Z

Test build #82272 has finished for PR 19330 at commit 80b6238.

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

SparkQA · 2017-09-28T19:20:03Z

Test build #82282 has finished for PR 19330 at commit a9d09f6.

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

jinxing64 · 2017-09-29T02:53:50Z

@kiszk
Thanks a lot for comments. Tests passed now. In current change ordered is included in jsonValue. But I'm not sure it is appropriate.

Thanks again for taking time looking into this :)

kiszk · 2017-09-29T03:38:02Z

@jinxing64 I would like to hear opinions from other reviewers whether ordered should be included in these methods like ToString, buildFormattedString, and jsonValue.
IMO, I think we should make these methods consistent, ordered is included or not. The method prints the status of MapType

SparkQA · 2017-11-15T15:07:33Z

Test build #83892 has finished for PR 19330 at commit 5445834.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2017-11-15T15:16:40Z

Test build #83893 has finished for PR 19330 at commit a472dae.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2017-11-16T06:32:30Z

Test build #83926 has finished for PR 19330 at commit b1886f6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2017-11-16T08:05:01Z

Test build #83929 has finished for PR 19330 at commit 6aa4b8b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

jinxing64 · 2017-11-16T09:21:54Z

Jenkins, retest this please.

SparkQA · 2017-11-16T12:23:02Z

Test build #83936 has finished for PR 19330 at commit 6aa4b8b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

maropu · 2018-01-09T01:52:10Z

Any update?

jinxing64 · 2018-01-09T04:18:04Z

@maropu @kiszk
In current change, ordered is excluded from toString, buildFormattedString, jsonValue; I prefer to keep ordered internal and used only when ordering.
Actually this change has been working in my cluster for a while.
Do you agree on the approach -- inject OrderMaps when needed ?
I'm happy to keep working on this, please comment.

maropu · 2018-01-11T00:45:48Z

Aha, I think we can't touch the stable interface (MapType) until the next release (v3.0). So, I think we need to hold this for a while. Or, we need to look for other approaches to fix this.

xxzzycq · 2018-02-27T07:06:12Z

Does the community currently have a join and group by code that supports mapType?

jinxing64 · 2018-02-27T07:29:25Z

@xxzzycq
Currently no

maropu · 2018-10-04T06:33:31Z

I think we can resume this pr for v3.0. Are u still there and can you resume this? @jinxing64
Also, can you add Closes #15970 in the PR description?

jinxing64 · 2018-10-11T02:29:55Z

@maropu
Thanks, and yes I'm still here and I can keep going if this pr is interested.
I will update this pr this weekend.

maropu · 2018-10-11T02:31:56Z

Thanks!

SparkQA · 2018-10-22T07:05:04Z

Test build #97739 has finished for PR 19330 at commit 6aa4b8b.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2018-10-22T11:51:46Z

Test build #97751 has finished for PR 19330 at commit 6aa4b8b.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2018-10-22T12:14:07Z

Test build #97767 has finished for PR 19330 at commit 6aa4b8b.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class Max(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Min(child: Expression) extends DeclarativeAggregate with OrderSpecified
case class Least(children: Seq[Expression]) extends Expression with OrderSpecified
case class Greatest(children: Seq[Expression]) extends Expression with OrderSpecified
case class OrderMaps(child: Expression) extends UnaryExpression with ExpectsInputTypes
class InterpretedOrdering(orders: Seq[SortOrder]) extends Ordering[InternalRow]
case class In(value: Expression, list: Seq[Expression]) extends Predicate with OrderSpecified
case class InSet(child: Expression, hset: Set[Any])
abstract class BinaryComparison extends BinaryOperator with Predicate with OrderSpecified

SparkQA · 2018-10-22T13:12:45Z

Test build #97817 has started for PR 19330 at commit 6aa4b8b.

jinxing64 commented Sep 23, 2017

View reviewed changes

jinxing64 force-pushed the SPARK-18134-v2 branch from 2e2b98d to bd50495 Compare September 23, 2017 06:58

jinxing64 force-pushed the SPARK-18134-v2 branch 2 times, most recently from 33e532b to 0385487 Compare September 26, 2017 04:57

jinxing64 changed the title ~~Orderable MapType~~ [SPARK-18134][SQL] Orderable MapType Sep 26, 2017

jinxing64 force-pushed the SPARK-18134-v2 branch from 0385487 to 5279338 Compare September 26, 2017 13:22

jinxing64 force-pushed the SPARK-18134-v2 branch from 5279338 to e071892 Compare September 27, 2017 13:36

kiszk reviewed Sep 27, 2017

View reviewed changes

jinxing64 force-pushed the SPARK-18134-v2 branch from 0b2b52c to 80b6238 Compare September 28, 2017 07:54

jinxing64 force-pushed the SPARK-18134-v2 branch from 80b6238 to a9d09f6 Compare September 28, 2017 16:22

jinxing64 force-pushed the SPARK-18134-v2 branch 2 times, most recently from 5445834 to a472dae Compare November 15, 2017 12:13

jinxing64 force-pushed the SPARK-18134-v2 branch from a472dae to b1886f6 Compare November 16, 2017 04:20

Orderable MapType

6aa4b8b

jinxing64 force-pushed the SPARK-18134-v2 branch from b1886f6 to 6aa4b8b Compare November 16, 2017 06:36

jinxing64 mentioned this pull request Oct 13, 2018

[SPARK-25724] Add sorting functionality in MapType. #22712

Closed

jinxing64 closed this Oct 28, 2018

maropu mentioned this pull request Mar 29, 2021

[SPARK-34819][SQL] MapType supports orderable semantics #31967

Closed

[SPARK-18134][SQL] Orderable MapType #19330

[SPARK-18134][SQL] Orderable MapType #19330

Uh oh!

Conversation

jinxing64 commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jinxing64 Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

jinxing64 commented Sep 23, 2017

Uh oh!

SparkQA commented Sep 23, 2017

Uh oh!

hvanhovell commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 23, 2017

Uh oh!

tejasapatil commented Sep 23, 2017

Uh oh!

jinxing64 commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 26, 2017

Uh oh!

SparkQA commented Sep 26, 2017

Uh oh!

jinxing64 commented Sep 26, 2017

Uh oh!

SparkQA commented Sep 26, 2017

Uh oh!

jinxing64 commented Sep 27, 2017

Uh oh!

kiszk commented Sep 27, 2017

Uh oh!

SparkQA commented Sep 27, 2017

Uh oh!

jinxing64 commented Sep 27, 2017

Uh oh!

SparkQA commented Sep 27, 2017

Uh oh!

kiszk commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 28, 2017

Uh oh!

SparkQA commented Sep 28, 2017

Uh oh!

SparkQA commented Sep 28, 2017

Uh oh!

jinxing64 commented Sep 29, 2017

Uh oh!

kiszk commented Sep 29, 2017

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

SparkQA commented Nov 16, 2017

Uh oh!

SparkQA commented Nov 16, 2017

Uh oh!

jinxing64 commented Nov 16, 2017

Uh oh!

SparkQA commented Nov 16, 2017

Uh oh!

maropu commented Jan 9, 2018

Uh oh!

jinxing64 commented Jan 9, 2018

Uh oh!

maropu commented Jan 11, 2018

Uh oh!

xxzzycq commented Feb 27, 2018

Uh oh!

jinxing64 commented Sep 23, 2017 •

edited

Loading

hvanhovell commented Sep 23, 2017 •

edited

Loading

jinxing64 commented Sep 24, 2017 •

edited

Loading

kiszk commented Sep 27, 2017 •

edited

Loading