Repository navigation
Conversation
|
Ready for review. |
| Map<Object, Object> fieldCache = new IdentityHashMap<>(array.length); | ||
| for (T item : array) { | ||
| if (item != null) { | ||
| fieldCache.put(item, getField(item, sortField)); |
There was a problem hiding this comment.
In the old code, getField() was called inside the comparator's try/catch, so an exception during field extraction was swallowed and treated as equal. However, the new code computes fields in a pre-loop with no exception handling.
This could be a problem for views like QueueView where getField() does server.locateQueue(...) and dereferences the result. Here if a queue is deleted mid-listing you get an NPE. Previously one bad element sorted as "equal" and the listing succeeded; now a single bad element throws out of getPagedResult, failing the entire management query.
There was a problem hiding this comment.
Thanks - added a try/catch in the loop.
| this.sortOrder = json.getString(SORT_ORDER); | ||
| } else if (json.containsKey(SORT_COLUMN)) { | ||
| this.sortField = json.getString(SORT_COLUMN); | ||
| this.sortOrder = json.getString(SORT_ORDER); |
There was a problem hiding this comment.
A payload with SORT_COLUMN but no SORT_ORDER now throws where the old code didn't.
There was a problem hiding this comment.
Fixed by adding a check if json contains SORT_ORDER.
| Arrays.sort(array, cachedComparator); | ||
|
|
||
| if (page == -1 || pageSize == -1) { | ||
| return Arrays.asList(array); |
There was a problem hiding this comment.
This now returns a mutable Arrays.asList(...) instead of Collections.unmodifiableList(...). This might not make any difference, but it's a contract change.
There was a problem hiding this comment.
Code now returns Collections.unmodifiableList(collectionList) or Collections.unmodifiableList(collectionList.subList(start, end)).
| if (sortOrderDescending) { | ||
| if (rightValue instanceof Comparable r) { | ||
| return r.compareTo(leftValue); | ||
| } | ||
| } else { | ||
| if (leftValue instanceof Comparable l) { | ||
| return l.compareTo(rightValue); | ||
| } |
There was a problem hiding this comment.
getField returns null for absent/optional fields, so a sort field with some null values is realistic.
Consider the scenario with ascending order, leftValue = abc, and rightValue = null:
- Old code:
rightValue instanceof Comparableisfalseso return0. Clean, no exception. - New code: only
leftValueis checked; it isComparable, so it calls"abc".compareTo(null)throwing aNullPointerExceptionwhich is caught so return 0.
The result (i.e., 0) is the same, but the new path throws and catches an exception on roughly every comparison that involves a null, inside the hottest loop of the sort. This is orders of magnitude more expensive than the avoided instanceof. So sorting any data containing a null will be slower, not faster.
There was a problem hiding this comment.
The new code now also checks for null values before executing the compareTo method:
if (leftValue == rightValue) {
return 0;
}
// push nulls to bottom of the list
if (leftValue == null) {
return 1;
}
if (rightValue == null) {
return -1;
}
Sort values can be null, e.g. the client ID of a connection without one. A comparator which treats null as equal to everything violates the Comparator contract, so sorting failed with "Comparison method violates its general contract!". Nulls must go to the bottom of the list in both sort orders. TimSort only detects contract violations with 32 or more elements and only for some orderings, so the test sorts 20 shuffled orderings of 64 elements before checking any result. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The values of one sort field can have different types. InVM connection IDs are Strings while Netty connection IDs are ChannelIds, so sorting connections by connectionID (the default for listConnections) compares a String with a ChannelId when both kinds of connections exist. Calling compareTo across such types throws ClassCastException, and treating them as equal violates the Comparator contract. testSortWithMixedValueTypes sorts 64 values mixing nulls, Integers, Strings and values which are not Comparable. Like the null value test it sorts 20 shuffled orderings in both directions before checking any result, then checks that nulls are last and that the other values are grouped by type and ordered within each type. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The comparator was changed - another function was added: According to claude, comparison between 2 different classes could happen, for example, when comparing ConnectionView - CONNECTION_ID (String vs io.netty.channel.DefaultChannelId):
This PR is now also connected to ARTEMIS-6321 - which was already solved here with the null checks before the latest push. Regarding the performance, I'm getting wildly different mem per op numbers for main(8e2d6ab) now.
Sorry if this became a bit of a mess. |
Benchmarks below: