Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HierarchicalTableImpl bug when replacing an inputed linkage directive with a real one #3404

Merged
merged 4 commits into from
Feb 3, 2023
Merged

Fix HierarchicalTableImpl bug when replacing an inputed linkage directive with a real one #3404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c99e95
Add sorting capability to IntrusiveArraySet
rcaudy Feb 3, 2023
1b67325
Fix HierarchicalTableImpl bug when replacing a key directive: we *als…
rcaudy Feb 3, 2023
e48ab12
Spotless, I stab at thee.
rcaudy Feb 3, 2023
88828f8
Make my assertion a little more idiomatic
rcaudy Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions Util/src/main/java/io/deephaven/util/datastructures/intrusive/IntrusiveArraySet.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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