From e62a8d5ad10d4364da1d70716b240aef7591694f Mon Sep 17 00:00:00 2001 From: Stamatis Zampetakis Date: Mon, 18 Nov 2024 16:41:48 +0100 Subject: [PATCH 1/2] [CALCITE-6698] Wrong (swapped) results in PartiallyOrderedSet#getNonChildren and getNonParents 1. Align the behavior of getNonChildren/getNonParents with: i) the class documentation; ii) the behavior of getParents/getChildren methods. 2. Add explicit Javadoc to clarify the results of its method. 3. Update and add test cases with expected results. --- .../calcite/util/PartiallyOrderedSet.java | 22 +++++++++-- .../calcite/util/PartiallyOrderedSetTest.java | 37 +++++++++++++++++-- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java b/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java index 94df762bc829..e7ff1dca09c6 100644 --- a/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java +++ b/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java @@ -90,6 +90,10 @@ public class PartiallyOrderedSet extends AbstractSet { * in the set. */ private final Node topNode; + /** + * Synthetic node to hold all nodes that have no children. It does not appear + * in the set. + */ private final Node bottomNode; /** @@ -552,7 +556,7 @@ public void out(StringBuilder buf) { // breadth-first search, to iterate over every element once, printing // those nearest the top element first final Set seen = new HashSet<>(); - final Deque unseen = new ArrayDeque<>(getNonChildren()); + final Deque unseen = new ArrayDeque<>(getNonParents()); while (!unseen.isEmpty()) { E e = unseen.pop(); buf.append(" "); @@ -681,12 +685,24 @@ private void closure(Function> generator, E e, ImmutableList.Buil } } + /** + * Returns all elements in the set that have no children. These elements are the leaf/minimal + * elements of the poset. + * + * @return a list of elements that have no children. + */ public List getNonChildren() { - return strip(topNode.childList); + return strip(bottomNode.parentList); } + /** + * Returns all elements in the set that have no parents. These elements are the roots/maximal + * elements of the poset. + * + * @return a list of elements that have no parents. + */ public List getNonParents() { - return strip(bottomNode.parentList); + return strip(topNode.childList); } @Override public void clear() { diff --git a/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java b/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java index 05a48fde57d2..334bb86d4f38 100644 --- a/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java +++ b/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java @@ -20,6 +20,7 @@ import java.util.AbstractList; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.LinkedHashSet; import java.util.List; @@ -110,8 +111,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { poset.add(abcd); printValidate(poset); assertThat(poset, hasSize(2)); - assertThat(poset.getNonChildren(), hasToString("['abcd']")); - assertThat(poset.getNonParents(), hasToString("['']")); + assertThat(poset.getNonChildren(), hasToString("['']")); + assertThat(poset.getNonParents(), hasToString("['abcd']")); final String ab = "'ab'"; poset.add(ab); @@ -158,8 +159,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { poset.add(b); printValidate(poset); - assertThat(poset.getNonChildren(), hasToString("['abcd']")); - assertThat(poset.getNonParents(), hasToString("['']")); + assertThat(poset.getNonChildren(), hasToString("['']")); + assertThat(poset.getNonParents(), hasToString("['abcd']")); assertThat(poset.getChildren(b), hasToString("['']")); assertEqualsList("['ab', 'bcd']", poset.getParents(b)); assertThat(poset.getChildren(b), hasToString("['']")); @@ -180,6 +181,32 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { assertEqualsList("['ab', 'abcd']", poset.getAncestors("'a'")); } + @Test void testGetNonParentsOnLteIntPosetReturnsMaxValue() { + PartiallyOrderedSet poset = + new PartiallyOrderedSet<>((i, j) -> i <= j, Arrays.asList(20, 30, 40)); + assertThat(poset.getNonParents(), hasToString("[40]")); + } + + @Test void testGetNonChildrenOnLteIntPosetReturnsMinValue() { + PartiallyOrderedSet poset = + new PartiallyOrderedSet<>((i, j) -> i <= j, Arrays.asList(20, 30, 40)); + assertThat(poset.getNonChildren(), hasToString("[20]")); + } + + @Test void testGetNonParentsOnGteIntPosetReturnsMinValue() { + PartiallyOrderedSet poset = + new PartiallyOrderedSet<>((i, j) -> i >= j, Arrays.asList(20, 30, 40)); + assertThat(poset.getNonParents(), hasToString("[20]")); + } + + @Test void testGetNonChildrenOnGteIntPosetReturnsMaxValue() { + PartiallyOrderedSet poset = + new PartiallyOrderedSet<>((i, j) -> i >= j, Arrays.asList(20, 30, 40)); + printValidate(poset); + System.out.println(poset.getChildren(20)); + assertThat(poset.getNonChildren(), hasToString("[40]")); + } + @Test void testPosetTricky() { final PartiallyOrderedSet poset = new PartiallyOrderedSet<>(STRING_SUBSET_ORDERING); @@ -195,6 +222,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { printValidate(poset); poset.add("'ab'"); printValidate(poset); + assertThat(poset.getNonChildren(), hasToString("['a', 'b']")); + assertThat(poset.getNonParents(), hasToString("['ac', 'ab']")); } @Test void testPosetBits() { From e579c8407a77855247eccdef804de86bab62e860 Mon Sep 17 00:00:00 2001 From: Stamatis Zampetakis Date: Tue, 19 Nov 2024 10:57:33 +0100 Subject: [PATCH 2/2] Revert to old behavior and update Javadoc to clarify the results of the methods --- .../calcite/util/PartiallyOrderedSet.java | 18 +++++------ .../calcite/util/PartiallyOrderedSetTest.java | 30 +++++++++---------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java b/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java index e7ff1dca09c6..0a307e1e41ca 100644 --- a/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java +++ b/core/src/main/java/org/apache/calcite/util/PartiallyOrderedSet.java @@ -556,7 +556,7 @@ public void out(StringBuilder buf) { // breadth-first search, to iterate over every element once, printing // those nearest the top element first final Set seen = new HashSet<>(); - final Deque unseen = new ArrayDeque<>(getNonParents()); + final Deque unseen = new ArrayDeque<>(getNonChildren()); while (!unseen.isEmpty()) { E e = unseen.pop(); buf.append(" "); @@ -686,23 +686,23 @@ private void closure(Function> generator, E e, ImmutableList.Buil } /** - * Returns all elements in the set that have no children. These elements are the leaf/minimal - * elements of the poset. + * Returns all elements in the set that are not children. These elements appear at the top of + * the poset, and they usually referred to as roots/maximal elements of the poset. * - * @return a list of elements that have no children. + * @return a list of elements that are not children. */ public List getNonChildren() { - return strip(bottomNode.parentList); + return strip(topNode.childList); } /** - * Returns all elements in the set that have no parents. These elements are the roots/maximal - * elements of the poset. + * Returns all elements in the set that are not parents. These elements appear at the bottom of + * the poset, and they usually referred to as leafs/minimal elements of the poset. * - * @return a list of elements that have no parents. + * @return a list of elements that are not parents. */ public List getNonParents() { - return strip(topNode.childList); + return strip(bottomNode.parentList); } @Override public void clear() { diff --git a/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java b/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java index 334bb86d4f38..45126b6d337f 100644 --- a/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java +++ b/core/src/test/java/org/apache/calcite/util/PartiallyOrderedSetTest.java @@ -111,8 +111,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { poset.add(abcd); printValidate(poset); assertThat(poset, hasSize(2)); - assertThat(poset.getNonChildren(), hasToString("['']")); - assertThat(poset.getNonParents(), hasToString("['abcd']")); + assertThat(poset.getNonChildren(), hasToString("['abcd']")); + assertThat(poset.getNonParents(), hasToString("['']")); final String ab = "'ab'"; poset.add(ab); @@ -159,8 +159,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { poset.add(b); printValidate(poset); - assertThat(poset.getNonChildren(), hasToString("['']")); - assertThat(poset.getNonParents(), hasToString("['abcd']")); + assertThat(poset.getNonChildren(), hasToString("['abcd']")); + assertThat(poset.getNonParents(), hasToString("['']")); assertThat(poset.getChildren(b), hasToString("['']")); assertEqualsList("['ab', 'bcd']", poset.getParents(b)); assertThat(poset.getChildren(b), hasToString("['']")); @@ -181,30 +181,28 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { assertEqualsList("['ab', 'abcd']", poset.getAncestors("'a'")); } - @Test void testGetNonParentsOnLteIntPosetReturnsMaxValue() { + @Test void testGetNonParentsOnLteIntPosetReturnsMinValue() { PartiallyOrderedSet poset = new PartiallyOrderedSet<>((i, j) -> i <= j, Arrays.asList(20, 30, 40)); - assertThat(poset.getNonParents(), hasToString("[40]")); + assertThat(poset.getNonParents(), hasToString("[20]")); } - @Test void testGetNonChildrenOnLteIntPosetReturnsMinValue() { + @Test void testGetNonChildrenOnLteIntPosetReturnsMaxValue() { PartiallyOrderedSet poset = new PartiallyOrderedSet<>((i, j) -> i <= j, Arrays.asList(20, 30, 40)); - assertThat(poset.getNonChildren(), hasToString("[20]")); + assertThat(poset.getNonChildren(), hasToString("[40]")); } - @Test void testGetNonParentsOnGteIntPosetReturnsMinValue() { + @Test void testGetNonParentsOnGteIntPosetReturnsMaxValue() { PartiallyOrderedSet poset = new PartiallyOrderedSet<>((i, j) -> i >= j, Arrays.asList(20, 30, 40)); - assertThat(poset.getNonParents(), hasToString("[20]")); + assertThat(poset.getNonParents(), hasToString("[40]")); } - @Test void testGetNonChildrenOnGteIntPosetReturnsMaxValue() { + @Test void testGetNonChildrenOnGteIntPosetReturnsMinValue() { PartiallyOrderedSet poset = new PartiallyOrderedSet<>((i, j) -> i >= j, Arrays.asList(20, 30, 40)); - printValidate(poset); - System.out.println(poset.getChildren(20)); - assertThat(poset.getNonChildren(), hasToString("[40]")); + assertThat(poset.getNonChildren(), hasToString("[20]")); } @Test void testPosetTricky() { @@ -222,8 +220,8 @@ private static boolean isBitSuperset(Integer e1, Integer e2) { printValidate(poset); poset.add("'ab'"); printValidate(poset); - assertThat(poset.getNonChildren(), hasToString("['a', 'b']")); - assertThat(poset.getNonParents(), hasToString("['ac', 'ab']")); + assertThat(poset.getNonChildren(), hasToString("['ac', 'ab']")); + assertThat(poset.getNonParents(), hasToString("['a', 'b']")); } @Test void testPosetBits() {