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
[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
[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