Partitioned table tests and fixed #9757
Merged
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
force-pushed
the
findepi/iceberg-convert-carefully
branch
2 times, most recently
from
5c41002 to
0664901
Compare
findepi
changed the title
losipiuk
reviewed
Oct 25, 2021
| " 'a_short_decimal', " + | ||
| " 'a_long_decimal', " + | ||
| " 'a_varchar', " + | ||
| // " 'a_varbinary', " + TODO (https://github.com/trinodb/trino/issues/9755) this yields incorrect query results |
There was a problem hiding this comment.
when those are fixed it would be (maybe) nice to merge testCreatePartitionedTable and testAllAvailableTypes and just paramtrize with boolean partitioned.
There was a problem hiding this comment.
yeah, maybe. the assertions will be different...
and since we already cover all iceberg types except "fixed", it won't add much more value
losipiuk
reviewed
Oct 25, 2021
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Outdated
Show resolved
Hide resolved
findepi
force-pushed
the
findepi/iceberg-convert-carefully
branch
2 times, most recently
from
4c33ce5 to
93f35a7
Compare
losipiuk
reviewed
Oct 25, 2021
| @@ -76,7 +75,7 @@ public static Object convertIcebergValueToTrino(Type icebergType, Object value) | |||
| if (icebergType instanceof Types.TimestampType) { | |||
| long epochMicros = (long) value; | |||
| if (((Types.TimestampType) icebergType).shouldAdjustToUTC()) { | |||
| return timestampTzFromMicros(epochMicros, TimeZoneKey.UTC_KEY); | |||
| return timestampTzFromMicros(epochMicros); | |||
There was a problem hiding this comment.
what does shouldAdjustToUTC mean here?
There was a problem hiding this comment.
i think it means "this is timestamptz, not timestamp type".
seems the two are conflated into Types.TimestampType class
losipiuk
approved these changes
Oct 25, 2021
findepi
force-pushed
the
findepi/iceberg-convert-carefully
branch
2 times, most recently
from
58de93f to
d43ec65
Compare
Since it's reused, it doesn't belong to `PartitionTable`. This also sorts the cases in the method.
* merge `testPartitionedTableWithNullValues` into `testCreatePartitionedTable`. This not only reduces test verbosity, but also improves coverage for predicate application. * synchronize `testAllAvailableTypes` and `testCreatePartitionedTable` -- use consistent column naming, use consistent style for SQL, use consistent test structure * cover all supported types in these tests * exercise time/timestamp microsecond precision
This also adds test coverage for `SHOW STATS` and `$partitions` for all types in Iceberg.
Do not assume that unrecognized type has same Iceberg and Trino representation, under all circumstances.
Before the change, partition key of `timestamp with time zone` type was returned in session zone, unlike regular data columns. This caused representability problems for values within session zone's DST change backwards. This commit makes partition values to be returned with UTC zone, just like regular data columns are.
findepi
force-pushed
the
findepi/iceberg-convert-carefully
branch
from
d43ec65 to
73e214b
Compare
|
previous CI run -- #8432 |
alexjo2144
approved these changes
Oct 25, 2021
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.
Includes
Convert from Iceberg to Trino representation strictly
Fills outstanding TODO in
PartitionedTable.convert(method moved)Fix handling of Iceberg timestamptz partition key
Before the change, partition key of
timestamp with time zonetype wasreturned in session zone, unlike regular data columns. This caused
representability problems for values within session zone's DST change
backwards. This commit makes partition values to be returned with UTC
zone, just like regular data columns are.
Fixes #9704
Fixes #9703