Fix Delta Lake insertion issue by setting UTC timezone for timestamp with timezone partition column #16878
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…with time zone type
github-actions
bot
added
delta-lake
ebyhr
reviewed
Apr 4, 2023
Sure, still working on this. |
albericgenius
force-pushed
the
deltalake
branch
2 times, most recently
from
4cc4d47 to
3456fc5
Compare
findinpath
reviewed
Apr 7, 2023
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/AbstractDeltaLakePageSink.java
Show resolved
Hide resolved
findinpath
reviewed
Apr 7, 2023
...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksInsertCompatibility.java
Show resolved
Hide resolved
…with timezone partition column
albericgenius
changed the title
|
@ebyhr and @findinpath Welcome for any comments |
|
@ebyhr and @findinpath I guess this is close to getting merged :) |
ebyhr
reviewed
May 16, 2023
Comment on lines
+186
to
+187
| if (position.getErrorIndex() == -1 && timestamp.length() == position.getIndex()) { | ||
| if (accessor.isSupported(NANO_OF_SECOND)) { |
There was a problem hiding this comment.
These conditions look hard to understand for me. Could you leave a code comment with the example?
|
|
||
| public final class DeltaLakeWriteUtils | ||
| { | ||
| private DeltaLakeWriteUtils() {} |
There was a problem hiding this comment.
What's the difference from HiveWriteUtils? Please leave a code comment.
| String tableName = "test_dl_partitioned_insert_timestampTZ" + randomNameSuffix(); | ||
| onTrino().executeQuery("" + | ||
| "CREATE TABLE delta.default." + tableName + | ||
| " (c1 INT, c2 TIMESTAMP WITH TIME ZONE)" + |
There was a problem hiding this comment.
Please add a test for partitioned by nested type with timestamp with time zone.
Comment on lines
+146
to
+147
| onTrino().executeQuery("INSERT INTO delta.default." + tableName + " VALUES (1, TIMESTAMP '2023-04-05 10:00:00.666+01:00')"); | ||
| onTrino().executeQuery("INSERT INTO delta.default." + tableName + " VALUES (2, TIMESTAMP '2023-04-06 10:00:00+01:00')"); |
There was a problem hiding this comment.
nit: No need to split into two INSERT statements.
| row( | ||
| 2, | ||
| Timestamp.valueOf("2023-04-06 09:00:00"))); | ||
| assertThat(onTrino().executeQuery("SELECT c1, CAST(c2 AS TIMESTAMP) FROM delta.default." + tableName + " ORDER BY c1 ASC")) |
| @@ -130,6 +131,37 @@ public void testPartitionedInsertCompatibility() | |||
| } | |||
| } | |||
|
|
|||
| @Test(groups = {DELTA_LAKE_DATABRICKS, DELTA_LAKE_OSS, PROFILE_SPECIFIC_TESTS}) | |||
| public void testPartitionedInsertTimestampTZCompatibility() | |||
There was a problem hiding this comment.
Suggested change
| public void testPartitionedInsertTimestampTZCompatibility() | |
| public void testPartitionedInsertTimestampWithTimeZoneCompatibility() |
| return partitionValues.build(); | ||
| } | ||
|
|
||
| public static Object getField(DateTimeZone localZone, Type type, Block block, int position) |
There was a problem hiding this comment.
All types in this method are required in Delta Lake connector?
Comment on lines
+189
to
+193
| else { | ||
| int picosOfMilli = block.getInt(position, SIZE_OF_LONG); | ||
| Instant instant = Instant.ofEpochMilli(millisUtc).plusNanos(picosOfMilli * 1000); | ||
| return new TimestampTZ(instant.getEpochSecond(), instant.getNano(), UTC_KEY.getZoneId()); | ||
| } |
There was a problem hiding this comment.
Is this condition used in Delta Lake connector?
|
@albericgenius Are you still working on this? |
|
Superseded by #18353 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…with time zone type
Description
Fix #16822
Additional context and related issues
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: