[jira] [Commented] (CALCITE-7556) NameMultimap.remove(key, value) leaves an empty key bucket behind

["Mihai Budiu (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-7556?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18088610#comment-18088610 ] 

Mihai Budiu commented on CALCITE-7556:
--------------------------------------

[~rickydong] please let us know whether the proposed fix correctly addresses the issue

> NameMultimap.remove(key, value) leaves an empty key bucket behind
> -----------------------------------------------------------------
>
>                 Key: CALCITE-7556
>                 URL: https://issues.apache.org/jira/browse/CALCITE-7556
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.41.0
>            Reporter: Ruiqi Dong
>            Priority: Major
>              Labels: pull-request-available
>
> *Summary*
> NameMultimap.remove(String, V) removes the value from the per-key list, but if that value was the last entry for the key, it does not remove the key itself from the backing NameMap. The object is left in an inconsistent state:
>  * range(key, ...) is empty
>  * containsKey(key, ...) is still true
>  * map() still contains the key with an empty list
> Although remove is marked @Experimental, the surrounding public API still exposes clear consistency expectations between remove, containsKey, range, and map. The current behavior breaks those expectations.
>  
> *Affected code*
> File: core/src/main/java/org/apache/calcite/util/NameMultimap.java
> {code:java}
> @Experimental
> public boolean remove(String key, V value) {
>   final List<V> list = map().get(key);
>   if (list == null) {
>     return false;
>   }
>   return list.remove(value);
> } {code}
>  
> *Reproducer*Add the following test to core/src/test/java/org/apache/calcite/util/UtilTest.java
> {code:java}
> @Test void testNameMultimapRemoveLastValueRemovesKey() {
>   final NameMultimap<Integer> map = new NameMultimap<>();
>   map.put("baz", 1);
>   assertTrue(map.remove("baz", 1));
>   assertThat(map.range("baz", true), hasSize(0));
>   assertThat(map.containsKey("baz", true), is(false));
>   assertThat(map.containsKey("BAZ", false), is(false));
>   assertThat(map.map(), aMapWithSize(0));
> } {code}
> Run:
> {code:java}
> ./gradlew :core:test \
>   --tests org.apache.calcite.util.UtilTest.testNameMultimapRemoveLastValueRemovesKey {code}
> Observed behavior:
> The key remains visible after the last value has been removed
> {code:java}
> Expected: is <false>
>      but: was <true> {code}
> The failure is triggered by
> {code:java}
> assertThat(map.containsKey("baz", true), is(false)); {code}
> Expected behavior:
> After removing the last value associated with a key, the multimap should no longer report that key as present.
>  
> This is a container state-consistency bug. The public query methods disagree about whether the key still exists, and the backing map retains an empty bucket that should have been deleted. The @Experimental annotation may lower compatibility expectations, but it does not make mutually inconsistent observable state acceptable.
>  



--
This message was sent by Atlassian Jira
(v8.20.10#820010)