You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2023/09/05 17:04:03 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #8500: Parquet: Move iceberg-parquet files to iceberg-core
ajantha-bhat opened a new pull request, #8500:
URL: https://github.com/apache/iceberg/pull/8500
- 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.
Fixes #8453
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#discussion_r1380176383
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -265,6 +266,18 @@ public static void serialize(final Serializable obj, final OutputStream outputSt
}
}
+ public static void assertThrows(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat closed pull request #8500: Core: Move `iceberg-parquet` files to `iceberg-core`
URL: https://github.com/apache/iceberg/pull/8500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#discussion_r1933730481
##########
build.gradle:
##########
@@ -770,33 +775,6 @@ project(':iceberg-orc') {
}
}
-project(':iceberg-parquet') {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat closed pull request #8500: Core: Move `iceberg-parquet` files to `iceberg-core`
URL: https://github.com/apache/iceberg/pull/8500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#issuecomment-2691756916
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 dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#discussion_r1933732242
##########
build.gradle:
##########
@@ -897,7 +874,9 @@ project(':iceberg-snowflake') {
}
dependencies {
- implementation project(':iceberg-core')
+ implementation(project(':iceberg-core')) {
+ exclude group: 'com.github.luben', module: 'zstd-jni'
+ }
Review Comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#issuecomment-1829224667
Going with alternative approach of writing stats from `iceberg-data` module and engines calling that.
Core API will not write stats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#discussion_r1380176383
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -265,6 +266,18 @@ public static void serialize(final Serializable obj, final OutputStream outputSt
}
}
+ public static void assertThrows(
Review Comment:
Needed for the testcase moved from parquet module.
I can directly use `Assertions.assertThatThrownBy` in those tests. But that will increase the diff size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Move `iceberg-parquet` files to `iceberg-core` [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8500:
URL: https://github.com/apache/iceberg/pull/8500#issuecomment-2697044257
Closing this as community is favoring[1] towards using parquet via reflection in core module instead of moving files from parquet to core.
[1] https://github.com/apache/iceberg/pull/12060
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org