-
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
Small ring chunk source cleanup #5502
Small ring chunk source cleanup #5502
Conversation
This reduces the type checking for ring table chunk sources; essentially, matching the expectations that we might need to copy around empty `Object[0]` as a substitute for the more specifically typed empty array. While I'm not the biggest fan of persisting this (ab)use of the type system, it is out of scope to actually try and plumb correct typing throughout the stack at this time. Fixes deephaven#5498
I guess this use of the type system, while (ab)using |
@@ -73,7 +73,13 @@ public AbstractRingChunkSource(@NotNull Class<T> type, int capacity) { | |||
} | |||
this.capacity = capacity; | |||
// noinspection unchecked | |||
ring = (ARRAY) Array.newInstance(type, capacity); | |||
ring = type.isArray() |
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 think the wider issue here is that this code is using reflection to determine the array type instead of using inheritance (and using Object[]
to hold non-primitive values).
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.
Are you certain you shouldn't be using Object[]
in more cases?
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.
Never mind, we figured out that we were dealing with weird upstream data.
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.