Skip to content

Commit

Permalink
Small ring chunk source cleanup (#5502)
Browse files Browse the repository at this point in the history
This was originally explored as a fix for #5498; ultimately, it was an upstream data issue that was solved in #5503. Along the way though, it was noted that ring chunk sources were 1) using reflection in places they didn't need to, and 2) using specifically typed arrays for generic types as opposed to other object chunks which use `Object[]` at the top layer. In the latest iteration of this, the construction reflection was removed (there is a new getLength check); but I'm apt to keep the more specifically typed arrays unless we know they are causing other issues. At a minimum, the specifically typed arrays helped us notice and fix #5503.
  • Loading branch information
devinrsmith authored and stanbrub committed May 17, 2024
1 parent 0ece8ef commit 1316717
Show file tree
Hide file tree
Showing 11 changed files with 47 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import java.io.Closeable;
import java.lang.reflect.Array;
import java.util.Objects;
import java.util.function.LongConsumer;

import static io.deephaven.engine.table.impl.AbstractColumnSource.USE_RANGES_AVERAGE_RUN_LENGTH;
Expand Down Expand Up @@ -67,13 +68,12 @@ abstract class AbstractRingChunkSource<T, ARRAY, SELF extends AbstractRingChunkS
protected final int capacity;
long nextRingIx;

public AbstractRingChunkSource(@NotNull Class<T> type, int capacity) {
public AbstractRingChunkSource(ARRAY ring) {
this.ring = Objects.requireNonNull(ring);
this.capacity = Array.getLength(ring);
if (capacity <= 0) {
throw new IllegalArgumentException("Capacity must be positive");
}
this.capacity = capacity;
// noinspection unchecked
ring = (ARRAY) Array.newInstance(type, capacity);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Byte> columnSource(int n) {
}

public ByteRingChunkSource(int capacity) {
super(byte.class, capacity);
super(new byte[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public static RingColumnSource<Character> columnSource(int n) {
}

public CharacterRingChunkSource(int capacity) {
super(char.class, capacity);
super(new char[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Double> columnSource(int n) {
}

public DoubleRingChunkSource(int capacity) {
super(double.class, capacity);
super(new double[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Float> columnSource(int n) {
}

public FloatRingChunkSource(int capacity) {
super(float.class, capacity);
super(new float[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Integer> columnSource(int n) {
}

public IntegerRingChunkSource(int capacity) {
super(int.class, capacity);
super(new int[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Long> columnSource(int n) {
}

public LongRingChunkSource(int capacity) {
super(long.class, capacity);
super(new long[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,33 @@
import io.deephaven.engine.rowset.RowSet;
import org.jetbrains.annotations.NotNull;

import java.lang.reflect.Array;
import java.util.Objects;

// Note: this is not being auto-generated ATM
final class ObjectRingChunkSource<T> extends AbstractRingChunkSource<T, Object[], ObjectRingChunkSource<T>> {
final class ObjectRingChunkSource<T> extends AbstractRingChunkSource<T, T[], ObjectRingChunkSource<T>> {
public static <T> RingColumnSource<T> columnSource(Class<T> type, int capacity) {
if (type.isPrimitive()) {
throw new IllegalArgumentException();
}
return new RingColumnSource<>(type, new ObjectRingChunkSource<>(type, capacity),
new ObjectRingChunkSource<>(type, capacity));
}

public static <T> RingColumnSource<T> columnSource(Class<T> type, Class<?> componentType, int capacity) {
if (type.isPrimitive()) {
throw new IllegalArgumentException();
}
return new RingColumnSource<>(type, componentType, new ObjectRingChunkSource<>(type, capacity),
new ObjectRingChunkSource<>(type, capacity));
}

public ObjectRingChunkSource(Class<T> type, int capacity) {
super(type, capacity);
private ObjectRingChunkSource(Class<T> type, int capacity) {
// In the general case, we can't know that Array.newInstance(Class<T>, ...) will result in T[]; for example,
// type=int.class (T=Integer) => int[] (not Integer[]). That said, we know that type is generic, and thus we
// know resulting array type is T[].
// noinspection unchecked
super((T[]) Array.newInstance(type, capacity));
}

@Override
Expand All @@ -42,8 +53,7 @@ T get(long key) {
throw new IllegalArgumentException(
String.format("Invalid key %d. available=[%d, %d]", key, firstKey(), lastKey()));
}
// noinspection unchecked
return (T) ring[keyToRingIndex(key)];
return ring[keyToRingIndex(key)];
}

@Override
Expand All @@ -60,14 +70,12 @@ private class FillerImpl extends Filler {

@Override
protected void copyFromRing(int srcRingIx, int destOffset) {
// noinspection unchecked
dest.set(destOffset, (T) ring[srcRingIx]);
dest.set(destOffset, ring[srcRingIx]);
}

@Override
protected void copyFromRing(int srcRingIx, int destOffset, int size) {
// noinspection unchecked
dest.copyFromTypedArray((T[]) ring, srcRingIx, destOffset, size);
dest.copyFromTypedArray(ring, srcRingIx, destOffset, size);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public static RingColumnSource<Short> columnSource(int n) {
}

public ShortRingChunkSource(int capacity) {
super(short.class, capacity);
super(new short[capacity]);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.deephaven.chunk.ObjectChunk;
import io.deephaven.chunk.ShortChunk;
import io.deephaven.chunk.attributes.Values;
import io.deephaven.engine.rowset.RowSequence;
import io.deephaven.engine.rowset.RowSequenceFactory;
import io.deephaven.engine.table.ChunkSink;
import io.deephaven.engine.table.WritableColumnSource;
Expand Down Expand Up @@ -291,8 +292,10 @@ public ColumnSource<?> getColumnSource() {

final WritableColumnSource<?> cs = ArrayBackedColumnSource.getMemoryColumnSource(
chunkData.size(), dataType, componentType);
try (final ChunkSink.FillFromContext ffc = cs.makeFillFromContext(chunkData.size())) {
cs.fillFromChunk(ffc, chunkData, RowSequenceFactory.forRange(0, chunkData.size() - 1));
try (
final ChunkSink.FillFromContext ffc = cs.makeFillFromContext(chunkData.size());
final RowSequence rs = RowSequenceFactory.forRange(0, chunkData.size() - 1)) {
cs.fillFromChunk(ffc, chunkData, rs);
}
return cs;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//
package io.deephaven.engine.table.impl.sources.ring;

import io.deephaven.chunk.ObjectChunk;
import io.deephaven.engine.context.ExecutionContext;
import io.deephaven.engine.rowset.RowSet;
import io.deephaven.engine.rowset.RowSetFactory;
Expand Down Expand Up @@ -70,6 +71,19 @@ public void blinkTableToRing() {
coprime(14, 25);
}

@Test
public void doubleArrayChunk() {
final Object[] objectArray = {
null,
new double[] {},
new double[] {42.42, 43.43}
};
final Table table = TableTools.newTable(TstUtils.columnHolderForChunk(
"DoubleArray", double[].class, double.class, ObjectChunk.chunkWrap(objectArray)));
final Table ring = RingTableTools.of(table, 32, true);
checkEquals(table, ring);
}

private static void coprime(int a, int b) {
if (!BigInteger.valueOf(a).gcd(BigInteger.valueOf(b)).equals(BigInteger.ONE)) {
throw new IllegalArgumentException("not coprime: " + a + ", " + b);
Expand Down

0 comments on commit 1316717

Please sign in to comment.