Allow Iceberg MV with partitioning transforms on timestamptz #16637
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
findepi
requested review from
electrum,
losipiuk,
raunaqmorarka,
ebyhr and
alexjo2144
findepi
force-pushed
the
findepi/mv-partitioning-improvements
branch
from
f9153a0 to
4664627
Compare
`storageSchemaName` is defined on class level, so the storage schema should be created in `@BeforeClass`, not within a test.
findepi
force-pushed
the
findepi/mv-partitioning-improvements
branch
from
4664627 to
6ca801c
Compare
losipiuk
reviewed
Mar 22, 2023
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionTransforms.java
Show resolved
Hide resolved
|
Build green (https://github.com/trinodb/trino/actions/runs/4481491874/jobs/7878270959?pr=16637). |
findepi
force-pushed
the
findepi/mv-partitioning-improvements
branch
from
a289513 to
0dbd356
Compare
ebyhr
approved these changes
Mar 23, 2023
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMaterializedViewTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java
Show resolved
Hide resolved
Allow creation of Iceberg Materialized Views partitioned with a temporal partitioning function on a `timestamp with time zone` column. In MVs, the `timestamp with time zone` columns are generally stored as text to preserve time zone information. However, this prevents use of temporal partitioning functions on these columns. The commit keeps `timestamp with time zone` columns with partitioning applied on them as `timestamp with time zone` in the storage table. An obvious downside to this approach is that the time zone information is erased and it is not known whether this aligns with user intention or not. A better solution would be to introduce a point-in-time type (#2273) to discern between the cases where time zone information is important (like Java's `ZonedDateTime`) from cases where only point-in-time matters (like Java's `Instant`).
findepi
force-pushed
the
findepi/mv-partitioning-improvements
branch
from
0dbd356 to
79c7c29
Compare
They were probably copied over from the preceding backtick test case.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java
Show resolved
Hide resolved
raunaqmorarka
approved these changes
Mar 24, 2023
alexjo2144
reviewed
Mar 24, 2023
| return new Object[][] { | ||
| {"year", "date", "DATE '2005-09-09'"}, | ||
| {"year", "timestamp(6)", "TIMESTAMP '2005-09-10 13:31:00.123456'"}, | ||
| {"year", "timestamp(6) with time zone", "TIMESTAMP '2005-09-10 13:00:00.123456 Europe/Warsaw'"}, |
There was a problem hiding this comment.
For the timestamp with time zone tests, can you add a second value that has the same time but in a different zone and show that works properly?
There was a problem hiding this comment.
Also, a test with precision > 6?
There was a problem hiding this comment.
Also, a test with precision > 6?
this is not supported
There was a problem hiding this comment.
can you add a second value that has the same time but in a different zone and show that works properly?
it wouldn't be any different. Europe/Warsaw is not test's JVM zone so it's not special
Closed
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.
Allow creation of Iceberg Materialized Views partitioned with a
temporal partitioning function on a
timestamp with time zonecolumn.In MVs, the
timestamp with time zonecolumns are generally stored astext to preserve time zone information. However, this prevents use of
temporal partitioning functions on these columns. The commit keeps
timestamp with time zonecolumns with partitioning applied on them astimestamp with time zonein the storage table.An obvious downside to this approach is that the time zone information
is erased and it is not known whether this aligns with user intention or
not. A better solution would be to introduce a point-in-time type
(#2273) to discern between the
cases where time zone information is important (like Java's
ZonedDateTime) from cases where only point-in-time matters (likeJava's
Instant).