deephaven / deephaven-core

Deephaven Community Core
Other
249 stars 79 forks source link

Incorrect or inconsistent ZonedDateTime handling in operations #5241

Open rcaudy opened 6 months ago

rcaudy commented 6 months ago

Firstly, the over-arching issues:

Secondly, the consistency issues:

Solutions:

  1. Remove ZonedDateTime support from maybeConvertToPrimitive. This standardizes on Java's definition of comparison and equality, but eliminates opportunities for optimization.
  2. Remove the restriction that we only convert to primitive when reversible. This requires extra work to figure out what our result time zone should be in many cases. If we have a join with mismatched ConvertibleTimeSource.Zoned.getZone() results, we need to error out or "pick a winner". Worse, if we have to convert back from an un-zoned long source, we need to pick a zone. DateTimeUtils.timeZone(), e.g. the system default?
  3. Keep maybeConvertToPrimitive the same. For joins, we should only convert if both sides are reinterpretable and have the same fixed zone.

~I think we should pick option (2), as that renders it less fraught to reinterpret between Instant and ZonedDateTime. Otherwise, this reinterpretation changes the meaning of equality, etc, for the data within the column.~

I think we should pick option (3). It means zone matters for equality and comparison. It keeps aggBy and sort correct with current optimizations. We could eventually optimize naturalJoin and join, but they are correct as-is. We would have to fix a bug in aj that might result in error messages or incorrect equality/comparability.

rcaudy commented 5 months ago

@lbooker42 We need to do two things:

  1. Fix aj to ensure that we only reinterpret one side if we can reinterpret both sides.
  2. Maybe reinterpret ZonedDataTime in naturalJoin and join IF we can reinterpret both sides.
  3. Probably never reinterpret ZonedDateTime in whereIn, unless we can refactor to know the set table and the source table at the same time.
rcaudy commented 1 month ago

https://github.com/deephaven/deephaven-core/pull/5780#discussion_r1684961442 We may need to revisit column stats for ZDTs.