-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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!: statistical functions should return null
when provided a vector of only null
values
#5606
Conversation
engine/table/src/main/java/io/deephaven/engine/table/impl/by/CharChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/FloatChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/FloatChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/FloatChunkedVarOperator.java
Show resolved
Hide resolved
return 0.5 * (copy[n / 2 - 1] + copy[n / 2]); | ||
else return copy[n / 2]; | ||
// NULL values sorted to beginning of the array. | ||
${pt.primitive}[] copy = sort(values.toArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal. The implementation of sort
is sub-optimal:
- We make one
toArray()
call internally. - Then we make an array of boxed values.
- Then we sort using a comparator.
- Then we make a third copy wherein we unbox the values.
Sorting on the vector at least let's us skip one of the copies.
We should write a separate ticket for updating the sort kernels in io.deephaven.engine.table.impl.sort
to have a version that does not bother with a valuesToPermute
chunk, and use that version for the implementation of sort
. This way we make exactly one copyToArray()
and we're done. Note copyToArray()
, not toArray()
: we mustn't sort our backing array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a look, and the stuff that jumped out at me is the stuff @rcaudy already commented on.
engine/table/src/main/java/io/deephaven/engine/table/impl/by/ByteChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/DoubleChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/IntChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/LongChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/ShortChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. We just need a resolution on the 2 array copies.
engine/table/src/main/java/io/deephaven/engine/table/impl/by/FloatChunkedVarOperator.java
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/CharChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/FloatChunkedVarOperator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but approved regardless.
Accepting PR suggestions Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
Accepting PR suggestions Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#239 |
Updated Numeric functions and tests:
null
values presentnull
values presentUpdated UpdateBy aggregations and tests:
Updated AggBy aggregations:
Will close #5564