[jira] [Work logged] (BEAM-13354) Java SDK Harness MultimapUserState not using structural value when comparing keys

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/BEAM-13354?focusedWorklogId=689719&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-689719 ]

ASF GitHub Bot logged work on BEAM-13354:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 03/Dec/21 01:49
            Start Date: 03/Dec/21 01:49
    Worklog Time Spent: 10m 
      Work Description: kileys commented on a change in pull request #16092:
URL: https://github.com/apache/beam/pull/16092#discussion_r761589631



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/state/MultimapUserState.java
##########
@@ -151,19 +150,89 @@ public void clear() {
   /*
    * Returns an iterables containing all distinct keys in this multimap.
    */
-  public Iterable<K> keys() {
+  public PrefetchableIterable<K> keys() {
     checkState(
         !isClosed,
         "Multimap user state is no longer usable because it is closed for %s",
         keysStateRequest.getStateKey());
     if (isCleared) {
-      return Collections.unmodifiableCollection(Lists.newArrayList(pendingAdds.keySet()));
+      List<K> keys = new ArrayList<>(pendingAdds.size());
+      for (Map.Entry<?, KV<K, List<V>>> entry : pendingAdds.entrySet()) {
+        keys.add(entry.getValue().getKey());
+      }
+      return PrefetchableIterables.concat(keys);
+    }
+
+    PrefetchableIterable<K> persistedKeys = getPersistedKeys();
+    Map<Object, K> pendingRemovesNow = new HashMap<>(pendingRemoves);

Review comment:
       Could we just use a set here?

##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/state/MultimapUserState.java
##########
@@ -65,13 +62,11 @@
   private boolean isClosed;
   private boolean isCleared;
   // Pending updates to persistent storage
-  private HashSet<K> pendingRemoves = Sets.newHashSet();
-  private HashMap<K, List<V>> pendingAdds = Maps.newHashMap();
-  // Map keys with no values in persistent storage
-  private HashSet<K> negativeCache = Sets.newHashSet();

Review comment:
       How come you removed this?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 689719)
    Time Spent: 0.5h  (was: 20m)

> Java SDK Harness MultimapUserState not using structural value when comparing keys
> ---------------------------------------------------------------------------------
>
>                 Key: BEAM-13354
>                 URL: https://issues.apache.org/jira/browse/BEAM-13354
>             Project: Beam
>          Issue Type: Bug
>          Components: sdk-java-harness
>    Affects Versions: 2.35.0
>            Reporter: Luke Cwik
>            Assignee: Luke Cwik
>            Priority: P2
>             Fix For: 2.36.0
>
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> The user state implementation is using non-structural value comparisons which doesn't work for certain types like byte[].
> https://github.com/apache/beam/blob/9b0032051b9fee83ac864fd70134915237ed2756/sdks/java/harness/src/main/java/org/apache/beam/fn/harness/state/MultimapUserState.java#L68



--
This message was sent by Atlassian Jira
(v8.20.1#820001)