-
Notifications
You must be signed in to change notification settings - Fork 80
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
Provide query library functions for computing differences and sort order #5151
Conversation
} | ||
|
||
return IntStream.range(0, values.intSize("sortIndex")) | ||
.boxed().sorted((i, j) -> ${pt.boxed}.compare(values.get(i),values.get(j)) ) |
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.
Two issues:
- add one space, delete one:
compare(values.get(i), values.get(j)))
- Here and everywhere in this file I have a concern.
the concern is that when applied to Float
(or Double
) this template will use Float.compare
(or Double.compare
) and in particular will not use a NaN-aware comparison.
this would make the sort inconsistent with the one used for object arrays, which does use the NullNanAwareComparator
The same NaN issue comes up when we use Arrays.sort()
in this file on float[]
and double[]
so I guess the first question is: do we care about NaN
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.
Resolved, but it came with boxing and copies.
…phaven-core into 5101_3413_diff_rank
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 have a couple of small comments and micro-nits.
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#173 |
Query library functions for:
Resolves #5101
Resolves #3413