[SPARK-25724] Add sorting functionality in MapType. #22712
Conversation
| } | ||
|
|
||
| private[this] class OrderedWrapper { | ||
| var isOrdered: Boolean = false |
There was a problem hiding this comment.
I prefer not to make this mutable if we can. That can be a source of some pretty weird errors if we move from an unordered to an ordered map. Why do you need this?
There was a problem hiding this comment.
Thanks for quick reply :)
Actually I'm not pretty sure about this.
If we do it like below
case class MapType(
keyType: DataType,
valueType: DataType,
valueContainsNull: Boolean,
ordered: Boolean)
The ordered will be spread to lots places in the code (especially in the ...match ... case ... ) and users can will also see it. But I think ordered is a pretty internal parameter/characteristic and only used when sorting map. So I try to make it private and lazy created.
|
Test build #97339 has finished for PR 22712 at commit
|
| case m: MapType if m.isOrdered && order.direction == Ascending => | ||
| m.interpretedOrdering.asInstanceOf[Ordering[Any]].compare(left, right) | ||
| case m: MapType if m.isOrdered && order.direction == Descending => | ||
| m.interpretedOrdering.asInstanceOf[Ordering[Any]].reverse.compare(left, right) |
There was a problem hiding this comment.
We need to care about this ordering direction? We just need comparable maps?
There was a problem hiding this comment.
Actually, this is not necessary, but just to make the logic complete.
#9718 did the same thing.
There was a problem hiding this comment.
You mean we can't remove this? If not necessary, better to remove it off.
|
BTW, how does hive implement comparable maps? |
Is it below piece of code ? |
5 participants
What changes were proposed in this pull request?
This is related to #19330.
As subtask of SPARK-18134, this PR proposes to add a functionality in
MapTypeto sortMapData, in which all entries are already sorted.How was this patch tested?
Tests added.