Skip to content

Conversation

@jamespfaulkner
Copy link

Hey folks,

This PR addresses the issue #12395

I've contributed a slightly modified version of the original PR addressing this issue.

The differences are:

  • This includes changes to PlannedDataReader to account for the deprecation of DataReader
  • timestamp-millis is handled independently of timestamp-micros to account for data files written outside of iceberg.

I tested this locally on a small avro based hive table that was migrated to iceberg and queried via a IcebergGenerics:read scan:

  • Using the existing main branch, it recreates the IllegalArgumentException: Unknown logical type: org.apache.hive.iceberg.org.apache.avro.LogicalTypes$TimestampMillis issue
  • Using the PR changes resulted in that Exception no longer being thrown, but the wrong DateTime was being returned.
  • Both issues appeared to be fixed with these changes.

return ChronoUnit.NANOS.addTo(EPOCH, nanosFromEpoch).toLocalDateTime();
}

public static LocalDateTime timestampFromMillis(long millisFromEpoch) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add tests for anything that's new to TestDateTimeUtil

}
return GenericWriters.timestampNanos();

case "timestamp-millis":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a test that exercises this path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants