Skip to content

Remove unnecessary date/time conversions in Delta stats reader #18125

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 5, 2023
Merged

Remove unnecessary date/time conversions in Delta stats reader #18125

merged 4 commits into from
Jul 5, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 4, 2023

No description provided.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jul 4, 2023
@findepi findepi requested a review from ebyhr July 4, 2023 10:58
@cla-bot cla-bot bot added the cla-signed label Jul 4, 2023
@findepi findepi mentioned this pull request Jul 4, 2023
Merged
@github-actions github-actions bot added the delta-lake Delta Lake connector label Jul 4, 2023
ebyhr
ebyhr approved these changes Jul 5, 2023
View reviewed changes
...c/main/java/io/trino/plugin/deltalake/transactionlog/checkpoint/CheckpointEntryIterator.java Outdated
if (!instant.atZone(UTC).toLocalDate().isBefore(START_OF_MODERN_ERA)) {
values.put(name, packDateTimeWithZone(instant.toEpochMilli(), UTC_KEY));
long epochMillis = (long) readNativeValue(TIMESTAMP_MILLIS, valuesBlock, i);
if (floorDiv(epochMillis, MILLISECONDS_PER_DAY) >= START_OF_MODERN_ERA_EPOCH_DAY) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (floorDiv(epochMillis, MILLISECONDS_PER_DAY) >= START_OF_MODERN_ERA_EPOCH_DAY) {
if (floorDiv(epochMillis, MILLISECONDS_PER_DAY) >= START_OF_MODERN_ERA_EPOCH_DAY) {

@ebyhr
Copy link
Member

ebyhr commented Jul 5, 2023

CI failure is relevant.

Error:  Failures: 
Error:    TestDeltaLakeMinioAndHmsConnectorSmokeTest>BaseDeltaLakeConnectorSmokeTest.testCheckpointWriteStatsAsStruct:1300->AbstractTestQueryFramework.assertQuery:339 Execution of 'actual' query 20230704_123156_00189_dbs5v failed: SHOW STATS FOR test_checkpoint_write_stats_as_struct_474i9vvzh7
Error:    TestDeltaLakeMinioAndHmsConnectorSmokeTest>BaseDeltaLakeConnectorSmokeTest.testStatsSplitPruningBasedOnSepCreatedCheckpointOnTopOfCheckpointWithJustStructStats:1557->BaseDeltaLakeConnectorSmokeTest.testCountQuery:1872 » QueryFailed Failed accessing transaction log for table: smoke_test.test_sep_checkpoint_stats_pruning_struct_stats_b8m37dknc3
Error:    TestSplitPruning.testParquetStatisticsPruning:344->testCountQuery:409->assertResultAndSplitCount:417->assertResultAndSplitCount:435->AbstractTestQueryFramework.assertQueryStats:555 » QueryFailed Failed accessing transaction log for table: tpch.parquet_struct_statistics
Error:    TestTransactionLogAccess.testParquetStructStatistics:669 » Trino Failed accessing transaction log for table: schema.parquet_struct_statistics
Error:    TestDeltaLakeFileBasedTableStatisticsProvider.testStatisticsParquetParsedStatistics:314->getTableStatistics:454 » Trino Failed accessing transaction log for table: db_name.parquet_struct_statistics
Error:    TestCheckpointWriter.testCheckpointWriteReadParquetStatisticsRoundtrip:340->readCheckpoint:499 » IllegalArgument Millis overflow: 2866410000000000
Error:    TestDeltaLakeFileStatistics.testParseParquetStatistics:123 » IllegalArgument Millis overflow: 31267645200000000

@findepi
Copy link
Member Author

findepi commented Jul 5, 2023

thanks @ebyhr for your review!

@findepi findepi merged commit 644eb8b into trinodb:master Jul 5, 2023
@findepi findepi deleted the findepi/remove-unnecessary-date-time-conversions-in-delta-stats-reader-d01ca4 branch July 5, 2023 09:02
@github-actions github-actions bot added this to the 421 milestone Jul 5, 2023
@colebow colebow mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr approved these changes

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector no-release-notes This pull request does not require release notes entry
Milestone
421
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @ebyhr