You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/11/04 03:28:45 UTC
[GitHub] [iceberg] chenjunjiedada opened a new pull request #1716: Read all row group metadata only when position column is projected
chenjunjiedada opened a new pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716
This is used to avoid reading all row group metadata if the position metadata column is not projected.
----------------------------------------------------------------
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.
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] rdblue commented on a change in pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#discussion_r518928262
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java
##########
@@ -102,7 +107,9 @@
long computedTotalValues = 0L;
for (int i = 0; i < shouldSkip.length; i += 1) {
BlockMetaData rowGroup = rowGroups.get(i);
- startRowPositions[i] = offsetToStartPos.get(rowGroup.getStartingPos());
+ if (offsetToStartPos != null) {
+ startRowPositions[i] = offsetToStartPos.get(rowGroup.getStartingPos());
Review comment:
I think this should use `offsetToStartPos.getOrDefault(0L)`.
----------------------------------------------------------------
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.
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] chenjunjiedada commented on a change in pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#discussion_r519064135
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java
##########
@@ -89,8 +91,11 @@
this.shouldSkip = new boolean[rowGroups.size()];
// Fetch all row groups starting positions to compute the row offsets of the filtered row groups
- Map<Long, Long> offsetToStartPos = generateOffsetToStartPos();
- this.startRowPositions = new long[rowGroups.size()];
+ Map<Long, Long> offsetToStartPos = null;
+ if (expectedSchema.findField(MetadataColumns.ROW_POSITION.fieldId()) != null) {
+ offsetToStartPos = generateOffsetToStartPos();
+ this.startRowPositions = new long[rowGroups.size()];
Review comment:
Make sense to me. Updated.
----------------------------------------------------------------
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.
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] rdblue commented on pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#issuecomment-723504427
Looks good now. Thanks @chenjunjiedada!
----------------------------------------------------------------
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.
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] rdblue commented on a change in pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#discussion_r518927434
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java
##########
@@ -135,11 +135,9 @@ private void advance() {
throw new RuntimeIOException(e);
}
- long rowPosition = rowGroupsStartRowPos[nextRowGroup];
+ model.setPageSource(pages, rowGroupsStartRowPos == null ? 0 : rowGroupsStartRowPos[nextRowGroup]);
Review comment:
I don't think this file should change. If the determination not to use correct start positions in made in `ReadConf`, then it should just set the position to 0 there. This can use whatever `rowGroupStartRowPos` contains.
----------------------------------------------------------------
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.
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] rdblue commented on a change in pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#discussion_r518929089
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java
##########
@@ -89,8 +91,11 @@
this.shouldSkip = new boolean[rowGroups.size()];
// Fetch all row groups starting positions to compute the row offsets of the filtered row groups
- Map<Long, Long> offsetToStartPos = generateOffsetToStartPos();
- this.startRowPositions = new long[rowGroups.size()];
+ Map<Long, Long> offsetToStartPos = null;
+ if (expectedSchema.findField(MetadataColumns.ROW_POSITION.fieldId()) != null) {
+ offsetToStartPos = generateOffsetToStartPos();
+ this.startRowPositions = new long[rowGroups.size()];
Review comment:
I think this should be defined for all cases so that `ParquetReader` doesn't need to check whether it is defined. In that case, it should still be a `final` variable.
The only change for this PR should be whether `offsetToStartPos` is an empty map or the result of `generateOffsetToStartPos()`. We may also want to do the `ROW_POSITION` check in that method instead of here, depending on what is cleaner.
----------------------------------------------------------------
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.
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] rdblue merged pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716
----------------------------------------------------------------
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.
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] chenjunjiedada commented on a change in pull request #1716: Read all row group metadata only when position column is projected
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #1716:
URL: https://github.com/apache/iceberg/pull/1716#discussion_r517078864
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java
##########
@@ -135,11 +135,9 @@ private void advance() {
throw new RuntimeIOException(e);
}
- long rowPosition = rowGroupsStartRowPos[nextRowGroup];
+ model.setPageSource(pages, rowGroupsStartRowPos == null ? 0 : rowGroupsStartRowPos[nextRowGroup]);
Review comment:
The `rowPosition` will be ignored if the position column is not projected.
----------------------------------------------------------------
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.
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