-
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
refactor!: Rearrange TypeUtils and ArrayTypeUtils so they can be shared with JS API #5780
Conversation
7e612ba
to
a8c9352
Compare
engine/table/src/test/java/io/deephaven/engine/util/TestTypeUtils.java
Outdated
Show resolved
Hide resolved
Util/src/main/java/io/deephaven/util/type/NumericTypeUtils.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.
Couple changes in behavior noted.
columnSource = ReinterpretUtils.instantToLongSource(table.getColumnSource(columnName)); | ||
} else if (type == ZonedDateTime.class) { | ||
statsFunc = new DateTimeChunkedStats(); | ||
columnSource = ReinterpretUtils.zonedDateTimeToLongSource(table.getColumnSource(columnName)); |
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.
Fun fact, this is going to work, but it has implications. That is, it assumes that all ZDTs that map to the same Instant are equal, which isn't so according to ZDT.equals(). Not a thing to address here, now, but certainly a concern.
Removed some unused methods, rewrote some implementations to avoid reflection where unnecessary, removed ObjectInputStream/ObjectOutputStream usage.
BREAKING CHANGES: Moved isBoxedNumeric, isNumeric, isBigNumeric to NumericTypeUtils, removed @IsDateTime, removed other reflective methods from TypeUtils and ArrayTypeUtils.
Partial #188