You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rustyconover (via GitHub)" <gi...@apache.org> on 2023/04/02 22:05:06 UTC
[GitHub] [iceberg] rustyconover opened a new pull request, #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
rustyconover opened a new pull request, #7267:
URL: https://github.com/apache/iceberg/pull/7267
Add an implementation for ParquetUtils.getSplitOffsets that takes an InputFile, without this function it required users to often add the Hadoop JARs to read the parquet file's metadata since ParquetIO isn't available to external callers of this module.
--
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] API: Add ParquetUtils.getSplitOffsets that takes an InputFile [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#issuecomment-2408258000
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] API: Add ParquetUtils.getSplitOffsets that takes an InputFile [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#issuecomment-2313786330
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
[GitHub] [iceberg] rustyconover commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1157695737
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,24 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(file))) {
+ ParquetMetadata md = reader.getFooter();
+ List<Long> splitOffsets = Lists.newArrayListWithExpectedSize(md.getBlocks().size());
+ for (BlockMetaData blockMetaData : md.getBlocks()) {
+ splitOffsets.add(blockMetaData.getStartingPos());
Review Comment:
Fixed in latest commit.
--
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] API: Add ParquetUtils.getSplitOffsets that takes an InputFile [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
URL: https://github.com/apache/iceberg/pull/7267
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1157680833
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,24 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
Review Comment:
What's the use case that benefits from this public method? I think we only get offsets in writers at the moment and we always have `ParquetMetadata` at that point.
--
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
[GitHub] [iceberg] rustyconover commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1157692510
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,24 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
Review Comment:
I'm adding files directly to a table, that isn't using a writer.
I already have existing Parquet files on S3.
To add those files I'm using `DataFile.builder()` combined with `table.newAppend().appendFile().commit()`. The problem is by not using a writer there is no way to obtain the split offsets without importing the Hadoop dependency to read the split offsets. By adding this method I can avoid importing the Hadoop dependency saying about 300mb in a JAR file.
--
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
[GitHub] [iceberg] rustyconover commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1179890604
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,19 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(file))) {
+ ParquetMetadata md = reader.getFooter();
+ return getSplitOffsets(md);
+ } catch (IOException e) {
+ throw new RuntimeIOException(e, "Failed to read footer of file: %s", file);
Review Comment:
Should we update the other function to also raise the same exception?
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1157681621
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,24 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(file))) {
+ ParquetMetadata md = reader.getFooter();
+ List<Long> splitOffsets = Lists.newArrayListWithExpectedSize(md.getBlocks().size());
+ for (BlockMetaData blockMetaData : md.getBlocks()) {
+ splitOffsets.add(blockMetaData.getStartingPos());
Review Comment:
Shall we just call the overloaded method once we have `ParquetMetadata`?
```
return getSplitOffsets(reader.getFooter());
```
--
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
[GitHub] [iceberg] rustyconover commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1157693395
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,24 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(file))) {
+ ParquetMetadata md = reader.getFooter();
+ List<Long> splitOffsets = Lists.newArrayListWithExpectedSize(md.getBlocks().size());
+ for (BlockMetaData blockMetaData : md.getBlocks()) {
+ splitOffsets.add(blockMetaData.getStartingPos());
Review Comment:
We could do that. I'll change the PR.
--
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
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #7267: API: Add ParquetUtils.getSplitOffsets that takes an InputFile
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#discussion_r1170689053
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java:
##########
@@ -222,6 +222,19 @@ private static MessageType getParquetTypeWithIds(
return ParquetSchemaUtil.addFallbackIds(type);
}
+ /**
+ * Returns a list of offsets in ascending order determined by the starting position of the row
+ * groups.
+ */
+ public static List<Long> getSplitOffsets(InputFile file) {
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(file))) {
+ ParquetMetadata md = reader.getFooter();
+ return getSplitOffsets(md);
+ } catch (IOException e) {
+ throw new RuntimeIOException(e, "Failed to read footer of file: %s", file);
Review Comment:
[minor] should we throw `java.io.UncheckedIOException` instead considering we have marked RuntimeIOException as deprecated ?
--
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] API: Add ParquetUtils.getSplitOffsets that takes an InputFile [iceberg]
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on PR #7267:
URL: https://github.com/apache/iceberg/pull/7267#issuecomment-2314105060
Can we still make this change?
--
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