fix(bigquery-jdbc): fix time getters

[logachev]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces new type coercions and optimizations in BigQueryBaseArray, BigQueryTypeCoercer, and BigQueryTypeCoercionUtility, along with corresponding integration tests. However, several critical issues were identified. The size == 1 optimization in BigQueryBaseArray can violate the JDBC contract when array elements are themselves arrays, and the array-to-array coercion in BigQueryTypeCoercer can cause runtime ClassCastExceptions. Additionally, multiple conversions in BigQueryTypeCoercionUtility (such as Timestamp to OffsetDateTime, ZonedDateTime, and Instant) incorrectly use toLocalDateTime(), introducing timezone shifts. Parsing methods like parseFieldValueToLocalDateTime and FieldValueToTimestamp are fragile and will fail on BigQuery TIMESTAMP values. Finally, localTimeToTime should be refactored to use legacy Calendar manipulation to ensure proper DST handling.

[gemini-code-assist]

The parseFieldValueToLocalDateTime method is highly fragile and will fail when parsing BigQuery TIMESTAMP values. If the column is a TIMESTAMP, fv.getStringValue() typically returns a string with a timezone suffix and multiple spaces (e.g., 2023-07-28 12:30:00 UTC). raw.replace(' ', 'T') replaces all spaces, resulting in 2023-07-28T12:30:00TUTC, which is invalid and will cause LocalDateTime.parse to throw a DateTimeParseException. Even if there was only one space, LocalDateTime.parse does not support timezone offsets/suffixes. Instead of checking raw.contains(" ") first, you should determine if the field is a DATETIME or a TIMESTAMP. Since DATETIME strings in BigQuery do not have timezone suffixes (and typically contain T or a single space), and TIMESTAMP values should be retrieved via fv.getTimestampValue(), you should refine this logic to avoid parsing timezone-annotated strings directly with LocalDateTime.parse.

Suggested change

	static LocalDateTime parseFieldValueToLocalDateTime(FieldValue fv) {
	String raw = fv.getStringValue();
	if (raw.contains("T") || raw.contains(" ")) {
	return LocalDateTime.parse(raw.replace(' ', 'T'));
	}
	long micros = fv.getTimestampValue();
	return Instant.EPOCH
	.plus(micros, ChronoUnit.MICROS)
	.atOffset(ZoneOffset.UTC)
	.toLocalDateTime();
	}
	static LocalDateTime parseFieldValueToLocalDateTime(FieldValue fv) {
	String raw = fv.getStringValue();
	if (raw.contains("T") || (raw.contains(" ") && !raw.endsWith("UTC") && !raw.endsWith("Z"))) {
	return LocalDateTime.parse(raw.replace(' ', 'T'));
	}
	long micros = fv.getTimestampValue();
	return Instant.EPOCH
	.plus(micros, ChronoUnit.MICROS)
	.atOffset(ZoneOffset.UTC)
	.toLocalDateTime();
	}

[gemini-code-assist]

Using Timestamp.valueOf(utcDateTime) is highly problematic because Timestamp.valueOf(LocalDateTime) interprets the given local date-time in the system default timezone, not in UTC. Since utcDateTime is obtained using ZoneOffset.UTC, converting it via Timestamp.valueOf will shift the actual point in time by the system's timezone offset. For example, if the system timezone is UTC-5, the resulting Timestamp will be shifted by 5 hours. To preserve the exact point in time (as mandated by the JDBC spec), you should use Timestamp.from(instant) or construct the Timestamp directly from the epoch milliseconds while preserving the nanosecond precision.

    @Override
    public Timestamp coerce(Long value) {
      Instant instant = Instant.EPOCH.plus(value, ChronoUnit.MICROS);
      return Timestamp.from(instant);
    }

[gemini-code-assist]

This change is highly error-prone and will break parsing for BigQuery TIMESTAMP values. BigQuery TIMESTAMP string representations typically contain a space (e.g., 2023-07-28 12:30:00 UTC). Under this new logic, because the string contains a space, it will enter the if block and call Timestamp.valueOf(rawValue). However: 1. Timestamp.valueOf does not support timezone suffixes like UTC and will throw an IllegalArgumentException. 2. Even if it did parse, Timestamp.valueOf interprets the time in the system default timezone, whereas BigQuery timestamps are in UTC, leading to incorrect timezone shifts. Please revert this to the previous logic where DATETIME (containing T) is handled separately from TIMESTAMP (which should be retrieved via fieldValue.getTimestampValue() and converted timezone-agnostically).

Suggested change

	if (rawValue.contains("T") || rawValue.contains(" ")) {
	return Timestamp.valueOf(rawValue.replace('T', ' '));
	} else {
	// It's a TIMESTAMP numeric string.
	long microseconds = fieldValue.getTimestampValue();
	Instant instant = Instant.EPOCH.plus(microseconds, ChronoUnit.MICROS);
	// Timezone-agnostic conversion preserving exact point in time as mandated by JDBC spec
	return Timestamp.from(instant);
	}
	return new LongToTimestamp().coerce(fieldValue.getTimestampValue());
	if (rawValue.contains("T")) {
	return Timestamp.valueOf(rawValue.replace('T', ' '));
	}
	return new LongToTimestamp().coerce(fieldValue.getTimestampValue());

[gemini-code-assist]

Converting Timestamp to OffsetDateTime using ts.toLocalDateTime().atOffset(ZoneOffset.UTC) is incorrect and introduces timezone shifts. ts.toLocalDateTime() converts the timestamp to the system default timezone. Attaching ZoneOffset.UTC to this local date-time without adjusting the offset will result in an incorrect instant. Please revert this to use ts.toInstant().atOffset(ZoneOffset.UTC) (or Instant.ofEpochMilli(ts.getTime()).atOffset(ZoneOffset.UTC)) to preserve the correct instant.

Suggested change

	.registerTypeCoercion(
	(Timestamp ts) -> ts.toLocalDateTime().atOffset(ZoneOffset.UTC),
	Timestamp.class,
	OffsetDateTime.class)
	.registerTypeCoercion(
	(Timestamp ts) -> ts.toInstant().atOffset(ZoneOffset.UTC),
	Timestamp.class,
	OffsetDateTime.class)

[gemini-code-assist]

Converting Timestamp to ZonedDateTime using ts.toLocalDateTime().atZone(ZoneOffset.UTC) is incorrect and introduces timezone shifts. Please use ts.toInstant().atZone(ZoneOffset.UTC) to preserve the correct instant.

Suggested change

	.registerTypeCoercion(
	(Timestamp ts) -> ts.toLocalDateTime().atZone(ZoneOffset.UTC),
	Timestamp.class,
	ZonedDateTime.class)
	.registerTypeCoercion(
	(Timestamp ts) -> ts.toInstant().atZone(ZoneOffset.UTC),
	Timestamp.class,
	ZonedDateTime.class)

[gemini-code-assist]

Converting Timestamp to Instant using ts.toLocalDateTime().atOffset(ZoneOffset.UTC).toInstant() is incorrect and introduces timezone shifts. Please revert this to simply ts.toInstant().

Suggested change

	.registerTypeCoercion(
	(Timestamp ts) -> ts.toLocalDateTime().atOffset(ZoneOffset.UTC).toInstant(),
	Timestamp.class,
	Instant.class)
	.registerTypeCoercion(
	(Timestamp ts) -> ts.toInstant(),
	Timestamp.class,
	Instant.class)

[gemini-code-assist]

This optimization for size == 1 is highly problematic and introduces a bug when the array elements themselves are arrays (for example, when targetClass is Object.class and the single element is an Object[] representing a struct, or when dealing with multi-dimensional arrays). If targetClass is Object.class and firstVal is Object[], firstVal.getClass().getComponentType().equals(targetClass) evaluates to true (Object.class == Object.class). The method will then return firstVal directly (an array of length N representing the struct's fields) instead of returning a wrapped array of length 1 containing firstVal as its single element. This violates the JDBC contract for Array.getArray(). Please remove this size == 1 shortcut.

    Object javaArray = Array.newInstance(targetClass, size);

[gemini-code-assist]

The condition (sourceClass.isArray() && targetClass.isArray()) is dangerous because it allows returning value directly even when the source array type is not assignable to the target array type (e.g., trying to coerce LocalTime[] to Time[]). Since targetClass.isAssignableFrom(sourceClass) is false in such cases, casting the returned value to T will inevitably throw a ClassCastException at runtime. If the intention is to support element-wise coercion for arrays of different component types, you should implement a proper array coercion mechanism that creates a new array of the target component type and coerces each element individually. Otherwise, this condition should be removed as targetClass.isAssignableFrom(sourceClass) already safely handles assignable array types.

      if (targetClass.isAssignableFrom(sourceClass)) {
        return (T) value;
      }

[gemini-code-assist]

According to the project's general rules, when converting or shifting timezone fields for JDBC types like java.sql.Time, legacy Calendar manipulation should be used instead of java.time APIs to ensure Daylight Saving Time (DST) is handled properly. Using lt.atDate(LocalDate.of(1970, 1, 1)).atZone(ZoneId.systemDefault()) can lead to DST or historical offset discrepancies. Please refactor this to use legacy Calendar manipulation.

  static Time localTimeToTime(LocalTime lt) {
    Calendar cal = Calendar.getInstance();
    cal.clear();
    cal.set(1970, Calendar.JANUARY, 1, lt.getHour(), lt.getMinute(), lt.getSecond());
    cal.set(Calendar.MILLISECOND, lt.getNano() / 1_000_000);
    return new Time(cal.getTimeInMillis());
  }

References

1. When converting or shifting timezone fields for JDBC types like java.sql.Time, use legacy Calendar manipulation instead of java.time APIs to ensure Daylight Saving Time (DST) is handled properly.