-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Core: Move iceberg-parquet files to iceberg-core
#8500
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.
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
Conversation
iceberg-parquet files to iceberg-core
| } | ||
| } | ||
|
|
||
| public static void assertThrows( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for the testcase moved from parquet module as I didn't move the duplicate class TestHelpers.java from parquet.
I can directly use Assertions.assertThatThrownBy in those tests. But that will increase the diff size.
|
Going with alternative approach of writing stats from |
- Since core module need to write stats in parquet format, to avoid circular dependency, move all the files from iceberg-parquet module to iceberg code. - `TestParquetReadProjection` used to duplicate the test code of iceberg-api module's `TestReadProjection`. Removed the duplicate class and instead directly extend the original class from iceberg-api module. - Update TestParquetReadProjection to skip empty struct testcases as only Avro readers supports it. The testcases are now common for both Avro and Parquet readers.
| } | ||
| } | ||
|
|
||
| project(':iceberg-parquet') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the module as keeping empty jar to support compatibility was unnecessary. Users who depend on iceberg-parquet would obviously depend on iceberg-core. So, no impact for class imports.
But the Users build files might have to update to remove iceberg-parquet dependency. That had to be done anyways whether in this version or other when we delete it. We can update release notes about this.
| implementation project(':iceberg-core') | ||
| implementation(project(':iceberg-core')) { | ||
| exclude group: 'com.github.luben', module: 'zstd-jni' | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a compile error about duplicate class.
> Task :iceberg-snowflake:checkClassUniqueness FAILED
Cause: baseline-class-uniqueness detected multiple jars containing identically named classes. Please resolve these problems, or run `./gradlew checkClassUniqueness --fix`to accept them:
# Danger! Multiple jars contain identically named classes. This may cause different behaviour depending on classpath ordering.
# Run ./gradlew checkClassUniqueness --fix to update this file
## runtimeClasspath
[com.github.luben:zstd-jni, net.snowflake:snowflake-jdbc]
- com.github.luben.zstd.Zstd
- com.github.luben.zstd.util.Native
- com.github.luben.zstd.util.ZstdVersion
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
Closing this as community is favoring[1] towards using parquet via reflection in core module instead of moving files from parquet to core. [1] #12060 |
Mailing list discussion:
https://lists.apache.org/thread/3f43y55s6h5wmo4y89wqk25pytrngt9c
iceberg-parquetfiles toiceberg-coreiceberg-parquetfiles in docs.iceberg-parquetmodule is removed.