Skip to content

Commit

Permalink
Fix HierarchicalTableImpl bug when replacing an inputed linkage direc…
Browse files Browse the repository at this point in the history 
…tive with a real one (#3404)

* Add sorting capability to IntrusiveArraySet

* Fix HierarchicalTableImpl bug when replacing a key directive: we *also* need to remove it from the parent's children
  • Loading branch information
@rcaudy
rcaudy authored Feb 3, 2023
1 parent 4e3d62a commit b93911b
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 26 deletions.
25 changes: 19 additions & 6 deletions Util/src/main/java/io/deephaven/util/datastructures/intrusive/IntrusiveArraySet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,14 @@
import org.jetbrains.annotations.NotNull;

import java.lang.reflect.Array;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.Set;
import java.util.*;

/**
* An intrusive set that uses an array for its backing storage.
*
* <p>
* You can insert, remove, or check for existence in O(1) time. Clearing the set is O(n); as we need to null out
* references.
*
* <p>
* If you attempt to perform an operation element which is not in this set, but is in another set with the same adapter;
* then you are going to have a bad time. Tread carefully.
*
Expand Down Expand Up @@ -157,6 +154,7 @@ public boolean retainAll(@NotNull Collection<?> collection) {
int destinationSlot = 0;

for (Object c : collection) {
// noinspection unchecked
final int slot = adapter.getSlot((T) c);
// check if it exists in our set
if (slot >= 0 && slot < size && storage[slot] == c) {
Expand All @@ -171,6 +169,7 @@ public boolean retainAll(@NotNull Collection<?> collection) {

// swap c and destination slot
storage[slot] = storage[destinationSlot];
// noinspection unchecked
storage[destinationSlot] = (T) c;

adapter.setSlot(storage[destinationSlot], destinationSlot);
Expand Down Expand Up @@ -203,6 +202,20 @@ public void clear() {
size = 0;
}

/**
* Sort the contents of this IntrusiveArraySet according to {@code c}. <em>Any</em> subsequent mutation may render
* this IntrusiveArraySet unsorted, as removes are done in O(1) time by swapping with the last element.
*
* @param c The {@link Comparator} to sort by
*/
public void sort(@NotNull final Comparator<? super T> c) {
final int s = size;
Arrays.sort(storage, 0, s, c);
for (int ii = 0; ii < s; ++ii) {
adapter.setSlot(storage[ii], ii);
}
}

/**
* Adapter interface for elements to be entered into the set.
*/
Expand Down
69 changes: 53 additions & 16 deletions Util/src/test/java/io/deephaven/util/datastructures/intrusive/TestIntrusiveArraySet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
*/
package io.deephaven.util.datastructures.intrusive;

import junit.framework.TestCase;
import org.jetbrains.annotations.NotNull;
import org.junit.Test;

import java.util.*;
import java.util.stream.Collectors;

public class TestIntrusiveArraySet extends TestCase {
private static class IntrusiveValue {
import static org.junit.Assert.*;

public class TestIntrusiveArraySet {
private static class IntrusiveValue implements Comparable<IntrusiveValue> {
private IntrusiveValue(int sentinel) {
this.sentinel = sentinel;
}
Expand All @@ -28,6 +31,11 @@ public String toString() {
", slot=" + slot +
'}';
}

@Override
public int compareTo(@NotNull final IntrusiveValue other) {
return Integer.compare(sentinel, other.sentinel);
}
}

private static class Adapter implements IntrusiveArraySet.Adapter<IntrusiveValue> {
Expand All @@ -43,6 +51,7 @@ public void setSlot(IntrusiveValue element, int slot) {
}
}

@Test
public void testSimple() {
final IntrusiveArraySet<IntrusiveValue> set = new IntrusiveArraySet<>(new Adapter(), IntrusiveValue.class);
final IntrusiveValue twentyThree = new IntrusiveValue(23);
Expand All @@ -63,10 +72,7 @@ public void testSimple() {
assertTrue(set.contains(values.get(1)));
assertTrue(set.containsAll(values));

final List<IntrusiveValue> copy = new ArrayList<>();
for (IntrusiveValue value : set) {
copy.add(value);
}
final List<IntrusiveValue> copy = new ArrayList<>(set);

assertEquals(values, copy);

Expand All @@ -75,12 +81,7 @@ public void testSimple() {
set.add(nineteen);
assertEquals(values.size(), set.size());

for (final Iterator<IntrusiveValue> it = set.iterator(); it.hasNext();) {
final IntrusiveValue next = it.next();
if (next.sentinel == 1 || next.sentinel == 7 || next.sentinel == 19) {
it.remove();
}
}
set.removeIf(next -> next.sentinel == 1 || next.sentinel == 7 || next.sentinel == 19);

assertFalse(set.contains(values.get(0)));
assertFalse(set.contains(values.get(4)));
Expand All @@ -94,19 +95,19 @@ public void testSimple() {
copy.clear();
copy.addAll(set);

copy.sort(Comparator.comparing(v -> v.sentinel));
copy.sort(Comparator.naturalOrder());
assertEquals(values, copy);

set.retainAll(values);

copy.clear();
copy.addAll(set);

copy.sort(Comparator.comparing(v -> v.sentinel));
copy.sort(Comparator.naturalOrder());
assertEquals(values, copy);

final List<IntrusiveValue> valueCopy = new ArrayList<>(values);
valueCopy.removeIf(x -> x.sentinel == 2 || x.sentinel < 13);
valueCopy.removeIf(x -> x.sentinel < 13);
valueCopy.add(valueCopy.get(0));
valueCopy.add(valueCopy.get(0));

Expand All @@ -122,4 +123,40 @@ public void testSimple() {

assertEquals(valueCopy, copy);
}

@Test
public void testSort() {
final IntrusiveArraySet<IntrusiveValue> set = new IntrusiveArraySet<>(new Adapter(), IntrusiveValue.class);
set.sort(Comparator.naturalOrder());

final List<IntrusiveValue> values = IntrusiveValue.make(78, 1, 22, 5, 4, 3, 66, 67, 0, 100);
set.ensureCapacity(values.size());
set.addAll(values);

assertArrayEquals(set.toArray(), values.toArray());

set.sort(Comparator.naturalOrder());
values.sort(Comparator.naturalOrder());

assertArrayEquals(set.toArray(), values.toArray());

set.remove(values.remove(2));
set.remove(values.remove(5));

set.sort(Comparator.naturalOrder());
values.sort(Comparator.naturalOrder());

assertArrayEquals(set.toArray(), values.toArray());

values.addAll(IntrusiveValue.make(101, 102, 100, 99, 65, 87, 2, 2, 4));
set.ensureCapacity(100);
set.addAll(values);

assertArrayEquals(set.toArray(), values.toArray());

set.sort(Comparator.naturalOrder());
values.sort(Comparator.naturalOrder());

assertArrayEquals(set.toArray(), values.toArray());
}
}
32 changes: 28 additions & 4 deletions ...able/src/main/java/io/deephaven/engine/table/impl/hierarchical/HierarchicalTableImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,11 @@ private static final class LinkedDirective {
*/
private final long rowKeyInParentUnsorted;

/**
* This LinkedDirective's slot in its parent's {@link #children}.
*/
private int slotInParent = -1;

/**
* The action to take for this directive. Will be {@link VisitAction#Undefined undefined} on construction; this
* is used to determine whether a directive was newly-created by a "compute if absent". Set to
Expand Down Expand Up @@ -848,8 +853,24 @@ private VisitAction getAction() {
return action;
}

private LinkedDirective addChild(@NotNull final LinkedDirective childDirective) {
(children == null ? children = new ArrayList<>() : children).add(childDirective);
private LinkedDirective addOrReplaceChild(
@NotNull final LinkedDirective childDirective,
@Nullable final LinkedDirective removedChildDirective) {
if (children == null) {
Assert.eqNull(removedChildDirective, "removedChildDirective");
children = new ArrayList<>();
}
if (removedChildDirective == null) {
childDirective.slotInParent = children.size();
children.add(childDirective);
} else {
Assert.eq(children.get(removedChildDirective.slotInParent),
"children.get(removedChildDirective.slotInParent)",
removedChildDirective, "removedChildDirective");
Assert.eq(removedChildDirective.action, "removedChildDirective.action", Linkage);
childDirective.slotInParent = removedChildDirective.slotInParent;
children.set(childDirective.slotInParent, childDirective);
}
return this;
}

Expand Down Expand Up @@ -1058,7 +1079,8 @@ private LinkedDirective linkKeyTableNodeDirectives(
// with different filtering or different input data (e.g. in the case of a saved key table).
continue;
}
removeOldChildren(linkedDirectives.put(nodeKey, linked), linkedDirectives);
LinkedDirective removed = linkedDirectives.put(nodeKey, linked);
removeOldChildren(removed, linkedDirectives);
linked.setAction(fromKeyTable.getAction());
// Find ancestors iteratively until we hit the root or a node directive that already exists
boolean needToFindAncestors = true;
Expand All @@ -1071,7 +1093,9 @@ private LinkedDirective linkKeyTableNodeDirectives(
needToFindAncestors = false;
} else if (parentNodeKeyFound) {
nodeKey = parentNodeKeyHolder.getValue();
linked = linkedDirectives.putIfAbsent(nodeKey, linkedDirectiveFactory).addChild(linked);
linked = linkedDirectives.putIfAbsent(nodeKey, linkedDirectiveFactory)
.addOrReplaceChild(linked, removed);
removed = null;
if (linkedDirectiveInvalid(linked)) {
failIfConcurrentAttemptInconsistent();
log.error().append("Could not find node for parent key=").append(NODE_KEY_FORMATTER, nodeKey)
Expand Down

0 comments on commit b93911b

Please sign in to comment.