Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create a MutableMap when accessing keySet and values in SparkMap #58

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Create a MutableMap when accessing keySet and values in SparkMap #58

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions transportable-udfs-spark/src/main/scala/com/linkedin/transport/spark/data/SparkMap.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,26 @@ case class SparkMap(private var _mapData: MapData,
}

override def keySet(): util.Set[StdData] = {
val keysIterator: Iterator[Any] = if (_mutableMap == null) {
new Iterator[Any] {
var offset : Int = 0

override def next(): Any = {
offset += 1
_mapData.keyArray().get(offset - 1, _keyType)
}

override def hasNext: Boolean = {
offset < SparkMap.this.size()
}
}
} else {
_mutableMap.keysIterator
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a mutable map on the read path seems excessive. I see that _map.keyArray() exposes getter methods. Can we build an iterator on top of this method ourselves? I think we would need to just maintain a current index inside the iterator and then increment it on next().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is definitely excessive in this scenario, but I don't think there will be keySet/values calls without some get(key) call which would create a mutable map anyways.

I can make the change to make another Iterator though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change to the iterator. It's a cleaner solution, thanks for suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Looks much better. Consider reducing duplicated code? Something like this?

  override def keySet(): util.Set[StdData] = {
    val keysIterator: Iterator[Any] = if (_mutableMap == null) {
      new Iterator[Any] {
        private val keyArray = _mapData.keyArray()
        var offset : Int = 0

        override def next(): Any = {
          offset += 1
          keyArray.get(offset -1, _keyType)
        }

        override def hasNext: Boolean = offset < SparkMap.this.size()
      }
    } else {
      _mutableMap.keysIterator
    }

    new util.AbstractSet[StdData] {
      override def iterator(): util.Iterator[StdData] = new util.Iterator[StdData] {

        override def next(): StdData = SparkWrapper.createStdData(keysIterator.next(), _keyType)

        override def hasNext: Boolean = keysIterator.hasNext
      }

      override def size(): Int = SparkMap.this.size()
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

acked and updated.


new util.AbstractSet[StdData] {

override def iterator(): util.Iterator[StdData] = new util.Iterator[StdData] {
private val keysIterator = if (_mutableMap == null) _mapData.keyArray().array.iterator else _mutableMap.keysIterator

override def next(): StdData = SparkWrapper.createStdData(keysIterator.next(), _keyType)

Expand All @@ -54,10 +70,26 @@ case class SparkMap(private var _mapData: MapData,
}

override def values(): util.Collection[StdData] = {
val valueIterator: Iterator[Any] = if (_mutableMap == null) {
new Iterator[Any] {
var offset : Int = 0

override def next(): Any = {
offset += 1
_mapData.valueArray().get(offset - 1, _valueType)
}

override def hasNext: Boolean = {
offset < SparkMap.this.size()
}
}
} else {
_mutableMap.valuesIterator
}

new util.AbstractCollection[StdData] {

override def iterator(): util.Iterator[StdData] = new util.Iterator[StdData] {
private val valueIterator = if (_mutableMap == null) _mapData.valueArray().array.iterator else _mutableMap.valuesIterator

override def next(): StdData = SparkWrapper.createStdData(valueIterator.next(), _valueType)

Expand Down