Create a MutableMap when accessing keySet and values in SparkMap

[raymondlam12]

Currently, accessing the keySet via: _mapData.keyArray().array.iterator or values via: _mapData.valueArray().array.iterator fails as its underlying type in Spark is UnsafeArrayData.

UnsafeArrayData does not allow accessing the raw array: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java#L108 .

Creating the mutable map solves this problem.

[shardulm94]

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().

[raymondlam12]

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.

[raymondlam12]

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

[shardulm94]

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()
    }
  }

[raymondlam12]

acked and updated.

[shardulm94]

I don't think this is the right fix. Arrays are ordered and so this check should be ordered. I understand that in this specific case, the output you are trying to test is that of the map_keys, map_values functions which are unordered. IMO, an ideal fix would be to pass the output of the function call through an array_sort UDF in the test case for those specific UDFs.

If we create the iterator ourselves instead of using a mutable map as mentioned in my comment above, do we still need this in the scope of the PR?

[raymondlam12]

Yeah this should not be the fix, but I thought it might be okay to bandaid it since the unit tests don't seem to be evolving for now.
In theory, doing something like Map.get() then Map.keySet() would break the unit test ordering since a mutable map would be created which uses a sorted set as its key collection (at least from what it seems).
I think there should be some checkUnordered method in the StdTester, but it is not necessary for this RB.

If we create our own iterator without the mutable map, this change is not necessary.

[raymondlam12]

I don't have authorization to merge this branch; could you merge it on my behalf Shardul?

[shardulm94]

@raymondlam12 Thanks for the PR! A new version should be published in the next couple of hours.