Github user jsoltren commented on the issue:

    https://github.com/apache/spark/pull/18395
  
    This looks fine to me. Though, I did have to ask Marcelo a number of 
questions along the way to clarify what all was going on here. Maybe some of 
the points I mention below should make their way back to the code in the form 
of comments. I might suggest a couple of minor code changes from the discussion 
below.
    
    The idea here is that, in the in memory store, arrays of Java primitive 
types can be used as keys. (Given an array, use the entire array as a key.) 
But, since primitive types are not Java objects, we need some code to make this 
happen. ArrayWrappers.java is the place where this happens. Each primitive type 
that is supported, needs its own implementation. Not all the primitive types 
are covered here.
    
    It is unfortunate that there is a bunch of duplicated code in 
ArrayWrappers.java. Marcelo tells me that Java does not support generic methods 
for primitive types, so, a bunch of code has to be copy-pasted. I don't see a 
good way around this.
    
    The testing in ArrapWrappersSuite.java seems adequate, though I'd like to 
see coverage for long[] here as well, and in the future, for any further 
primitive types that are supported in ArrayWrappers.
    
    The core of the InMemoryStore implementation is an implementation of 
KVStore that wraps operations on an InstanceList, which itself wraps operations 
on a ConcurrentMap. This seems like a reasonable choice for this application.
    
    I don't think there is much that we can do about this, but, this seems like 
a lot of code for a nice presentation of a ConcurrentMap. I think this is just 
Java, since we need an Iterator, a View, and an inner class to hold records.
    
    I liked the use of a Java 1.8 anonymous function in InMemoryStore.write().
    
    All the tests pass when run locally.
    
    This change only defines the in-memory KVStore implementation. It does not 
wire it in to anything.
    
    Nits:
    
    IntelliJ had a bunch of warnings about replacing for with foreach, and 
about making public methods package-private.
    
    ArrayWrappers.java: 
    IntelliJ claims the following import statement appears to be unused:
    import com.google.common.base.Objects;
    Line 26: I would do s/used as a key/used as keys/.
    
    InMemoryStore.java:
    IntelliJ claims the following import statements appear to be unused:
    import java.lang.reflect.Array;
    import java.util.Arrays;
    import java.util.Comparator;
    import java.util.TreeMap;


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to