fix: document and update WritableChunk and CompactKernel #5920
Conversation
The WritableChunk documentation and implementation was update to more accurately reflect the desired behavior. A "fix-up" stage for floating point types was introduced. A sortUnsafe method was introduced to skip the "fix-up" stage for callers who do not need the "fix-up". The CompactKernel documentation and implementation was updated to more accurately reflect the desired behavior. The add-only min max operators were updated to ignore NaN values. Fixes deephaven#5824
devinrsmith
added
NoDocumentationNeeded
ReleaseNotesNeeded
|
Nightlies passed |
engine/chunk/src/main/java/io/deephaven/chunk/WritableChunk.java
Outdated
Show resolved
Hide resolved
engine/chunk/src/main/java/io/deephaven/chunk/WritableChunkImpl.java
Outdated
Show resolved
Hide resolved
engine/chunk/src/main/java/io/deephaven/chunk/WritableChunkImpl.java
Outdated
Show resolved
Hide resolved
engine/chunk/src/main/java/io/deephaven/chunk/WritableChunkImpl.java
Outdated
Show resolved
Hide resolved
engine/chunk/src/main/java/io/deephaven/chunk/WritableChunkImpl.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/util/compact/CompactKernelImpl.java
Outdated
Show resolved
Hide resolved
| } else if (DoubleComparisons.gt(candidate, value)) { | ||
| value = candidate; | ||
| } | ||
| if (MinMaxHelper.isNullOrNan(candidate)) { |
There was a problem hiding this comment.
Let's make sure with the group that we don't want NaN poisoning or NaNs last.
|
|
||
| import io.deephaven.util.QueryConstants; | ||
|
|
||
| final class MinMaxHelper { |
There was a problem hiding this comment.
I can justify removing CompactKernelImpl and reverting to the previous code for the CompactKernel replication b/c it already exists, but I can't justify writing new non-trivial replication code. I'm happy to move isNullOrNan into some common place like DoubleComparisons (or somewhere else accessible) if you prefer.
There was a problem hiding this comment.
I think the example provided by the compaction kernels shows that this case is trivial, unless I'm mistaken.
| @@ -509,40 +509,40 @@ public static Object maxObj(ObjectVector values) { | |||
| } | |||
|
|
|||
| public static double minDouble(DoubleVector values) { | |||
There was a problem hiding this comment.
We should remove this and use Numeric when we're sure Numeric is correct.
There was a problem hiding this comment.
@rcaudy I interpret this comment as "please add a TODO and ticket" not that we should adjust this for the current PR? Is that your intended meaning?
| final Table x = TableTools.newTable(TableTools.charCol(S1, source)); | ||
| x.setRefreshing(true); | ||
| return x.maxBy(); |
There was a problem hiding this comment.
This only tests the instantiation (addChunk) case. It doesn't test removal or modification handling. That said, we have adequate testing to compare recomputed results to updated results.
There was a problem hiding this comment.
Seems like a lot of these new assertions aren't being used yet. Should we demonstrate their utility and correctness by using them?
| private char min(char a, char b) { | ||
| return CharComparisons.lt(a, b) ? a : b; | ||
| private static char min(char a, char b) { | ||
| return CharComparisons.leq(a, b) ? a : b; |
There was a problem hiding this comment.
I'm curious as to why this change; I would expect that they are mostly equivalent; but that maybe we are biasing towards keeping the first value instead?
| @@ -509,40 +509,40 @@ public static Object maxObj(ObjectVector values) { | |||
| } | |||
|
|
|||
| public static double minDouble(DoubleVector values) { | |||
There was a problem hiding this comment.
@rcaudy I interpret this comment as "please add a TODO and ticket" not that we should adjust this for the current PR? Is that your intended meaning?
| final IteratorAssert<Long> itAssert = assertThat(it); | ||
| for (int i = 0; i < out.length; ++i) { | ||
| itAssert.hasNext(); | ||
| out[i] = prev ? source.getPrevChar(it.nextLong()) : source.getChar(it.nextLong()); |
There was a problem hiding this comment.
So in the other cases we are using chunks, but not this case. What is the reasoning?
| } | ||
|
|
||
| @Override | ||
| public void testEmptyChar() {} |
There was a problem hiding this comment.
Why are the rest of the tests empty here?
| } | ||
|
|
||
| @Override | ||
| public void testEmptyChar() {} |
| } | ||
|
|
||
| public void check(char expected, char[] data) { | ||
| assertThat(sortedFirst(data)) |
There was a problem hiding this comment.
TODO: This test should have a sentinel value too, not just the single value under test.
The WritableChunk documentation and implementation was update to more accurately reflect the desired behavior. A "fix-up" stage for floating point types was introduced. A sortUnsafe method was introduced to skip the "fix-up" stage for callers who do not need the "fix-up".
The CompactKernel documentation and implementation was updated to more accurately reflect the desired behavior.
The add-only min max operators were updated to ignore NaN values.
Fixes #5824