[GitHub] StefanRRichter commented on issue #7226: FLINK-11050 add lowerBound and upperBound for optimizing RocksDBMapState's entries

[GitBox <gi...@apache.org>]

StefanRRichter commented on issue #7226: FLINK-11050 add lowerBound and upperBound for optimizing RocksDBMapState's entries
URL: https://github.com/apache/flink/pull/7226#issuecomment-446148429
 
 
   I think this suggestion is problematic for the following reasons:
   1. Map state is in general unordered
   2. Keys in map state are not required to be `Comparable`, so what defines the order against which we can compare upper and lower keys?
   3. Even if keys implement, `Comparable`, their order in RocksDB is depending on the lexicographical order of there bytes in serialized form, which can be a differnt order from what can be defined in the `compareTo()`
   4. The method is doing different things for RocksDB and Heap right now and this is not properly reflecting in the documentation of the method.
   
   Overall that leads me to the conclusion that we cannot rush to add this optimization but need a bit more careful thinking, e.g. introducing a subclass OrderedMapState (openly or hidden and cast where optimization is required). Even in that case we need to be careful when addressing the problem of different orders (`Comparable` vs byte-lexicographical) and I feel that needs more thought. So currently I am leaning towards 👎 for the suggested approach.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services