Change Iceberg to use the new ParquetFileWriter #4424
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
Do you have an example collision in mind? |
With the code in my PR, I saw pretty frequent failures in TestHiveCreateExternalTable accessing table "test_create_external" in ci automated testing. I was unable to reproduce the problem on my laptop. The failures in ci automated testing haven't happened with the tables renamed, and the renaming seems entirely harmless to me. It's not a very big statistical universe, and perhaps what I observed was just chance and the failures will resurface. @electrum also wondered how a naming conflict could have caused the test to fail. |
findepi
reviewed
Jul 16, 2020
| @@ -54,22 +54,22 @@ public void testCreateExternalTableWithData() | |||
| File tableLocation = new File(tempDir, "data"); | |||
|
|
|||
| @Language("SQL") String createTableSql = format("" + | |||
| "CREATE TABLE test_create_external " + | |||
| "CREATE TABLE test_create_external_with_data " + | |||
There was a problem hiding this comment.
Rename test databases to prevent name collisions
-> Rename test tables ...
There was a problem hiding this comment.
Doh, good point - - It was the tables that were renamed.
I just rebased this PR on tip master, since there were conflicts and removed the commit that renamed the tables. If the Hive test failures recur, I'll submit a separate PR with the renames.
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
2 times, most recently
from
5a46032 to
a3cfb02
Compare
electrum
reviewed
Jul 20, 2020
presto-hive/src/main/java/io/prestosql/plugin/hive/parquet/ParquetFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/HiveParquetPrimitiveTypeConverter.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/HiveParquetPrimitiveTypeConverter.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/HiveParquetPrimitiveTypeConverter.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/ParquetSchemaConverter.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/ParquetSchemaConverter.java
Outdated
Show resolved
Hide resolved
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
from
a3cfb02 to
afe026c
Compare
|
Thanks very much for the detailed review, @electrum! I just force-pushed an update to the PR that acts on all your comments. |
electrum
reviewed
Jul 20, 2020
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveParquetPrimitiveTypeConverter.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
from
afe026c to
924874c
Compare
|
I force-pushed fixes for your remaining comments @electrum |
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
from
924874c to
9d8cd35
Compare
alexjo2144
reviewed
Jul 21, 2020
...-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergParquetPrimitiveTypeConverter.java
Outdated
Show resolved
Hide resolved
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
2 times, most recently
from
4349e93 to
d247c41
Compare
electrum
approved these changes
Jul 21, 2020
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergPageSink.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergParquetFileWriter.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergRecordFileWriter.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/PrimitiveTypeMapBuilder.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/PrimitiveTypeMapBuilder.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/PrimitiveTypeMapBuilder.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/ParquetWriter.java
Outdated
Show resolved
Hide resolved
presto-parquet/src/main/java/io/prestosql/parquet/writer/ParquetWriters.java
Outdated
Show resolved
Hide resolved
This commit changes the Iceberg connector to use the new ParquetFileWriter, and fixes bugs in handling of decimal and timestamp columns for both Orc and Parquet. This commit removes DomainConverter and the test TestDomainConverter, superseded by fixes and conversions in ExpressionConverter, MessageTypeConverter and new class TimestampValueWriter. This commit adds new TestIcebergSmoke tests for decimal and timestamp, and fixes the existing tests. With this commit, all TestIcebergSmoke tests pass.
djsstarburst
force-pushed
the
david.stryker/iceberg-new-parquet-writer-redux
branch
from
d247c41 to
81db7fe
Compare
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.
This commit changes the Iceberg connector to use the new ParquetFileWriter, and fixes bugs in handling of decimal and timestamp columns for both Orc and Parquet.
This commit removes DomainConverter and the test TestDomainConverter, superseded by fixes and conversions in ExpressionConverter, MessageTypeConverter and new class TimestampValueWriter.
This commit adds new TestIcebergSmoke tests for decimal and timestamp, and fixes the existing tests. With this commit, all TestIcebergSmoke tests pass.
#1324