You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2023/08/30 18:56:58 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request, #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
aokolnychyi opened a new pull request, #8432:
URL: https://github.com/apache/iceberg/pull/8432
This PR fixes skipped file counts in `ManifestReader` with deleted entries as discussed [here](https://github.com/apache/iceberg/pull/8123#discussion_r1275561171).
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310701022
##########
core/src/test/java/org/apache/iceberg/TestScanPlanningAndReporting.java:
##########
@@ -236,9 +241,9 @@ public void scanningWithSkippedDataFiles() throws IOException {
assertThat(result.resultDeleteFiles().value()).isEqualTo(0);
assertThat(result.scannedDataManifests().value()).isEqualTo(1);
assertThat(result.scannedDeleteManifests().value()).isEqualTo(0);
- assertThat(result.skippedDataManifests().value()).isEqualTo(1);
+ assertThat(result.skippedDataManifests().value()).isEqualTo(2);
Review Comment:
The overwrite rewrites the original manifest and adds a new one for the new data 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] aokolnychyi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310696656
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
I had to add `hasRowFilter()` and `hasPartitionFilter()` to avoid cyclomatic complexity warnings. I also think it makes it more readable.
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310696656
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
I had to add `hasRowFilter()` and `hasPartitionFilter()` to avoid cyclomatic complexity warnings.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312400631
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do you need to consider the condition?
```
partitionSet.isEmpty()
```
How about adding another method `hasPartitionSet`?
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312411625
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
partitionSet.isEmpty()
```
If the partitionSet is empty, we seems to filter all entries. Maybe we can return empty entries directly when the partitionSet is empty. I'm not sure whether we have guaranteed the partitionSet is not empty. If I'm wrong, you can point it out.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312400631
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
!partitionSet.isEmpty()
```
--
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] rdblue commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1313606655
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -266,12 +280,16 @@ private CloseableIterable<ManifestEntry<F>> open(Schema projection) {
}
CloseableIterable<ManifestEntry<F>> liveEntries() {
- return CloseableIterable.filter(
- content == FileType.DATA_FILES
- ? scanMetrics.skippedDataFiles()
- : scanMetrics.skippedDeleteFiles(),
- entries(),
- entry -> entry != null && entry.status() != ManifestEntry.Status.DELETED);
+ return entries(true /* only live entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> filterLiveEntries(
Review Comment:
Minor: these last two methods could be static.
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310698115
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Evaluator evaluator = evaluator();
InclusiveMetricsEvaluator metricsEvaluator = metricsEvaluator();
// ensure stats columns are present for metrics evaluation
boolean requireStatsProjection = requireStatsProjection(rowFilter, columns);
Collection<String> projectColumns =
requireStatsProjection ? withStatsColumns(columns) : columns;
+ CloseableIterable<ManifestEntry<F>> entries =
+ open(projection(fileSchema, fileProjection, projectColumns, caseSensitive));
return CloseableIterable.filter(
content == FileType.DATA_FILES
? scanMetrics.skippedDataFiles()
: scanMetrics.skippedDeleteFiles(),
- open(projection(fileSchema, fileProjection, projectColumns, caseSensitive)),
+ onlyLive ? filterLiveEntries(entries) : entries,
Review Comment:
The deleted entries must be discarded prior to evaluating stats primarily to have correct skipped counts. It should also help the performance.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312411625
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition
```
!partitionSet.isEmpty()
```
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
!partitionSet.isEmpty()
```
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312411625
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
partitionSet.isEmpty()
```
If the partitionSet is empty, we seems to filter all entries. Maybe we can return empty entries directly when the partitionSet is empty. I'm not sure whether we have guaranteed the partitionSet is not empty. Correct me if I'm wrong.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312411625
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
partitionSet.isEmpty()
```
If the partitionSet is empty, we seems to filter out all entries. Maybe we can return empty entries directly when the partitionSet is empty. I'm not sure whether we have guaranteed the partitionSet is not empty. Correct me if I'm wrong.
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1313632181
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -266,12 +280,16 @@ private CloseableIterable<ManifestEntry<F>> open(Schema projection) {
}
CloseableIterable<ManifestEntry<F>> liveEntries() {
- return CloseableIterable.filter(
- content == FileType.DATA_FILES
- ? scanMetrics.skippedDataFiles()
- : scanMetrics.skippedDeleteFiles(),
- entries(),
- entry -> entry != null && entry.status() != ManifestEntry.Status.DELETED);
+ return entries(true /* only live entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> filterLiveEntries(
Review Comment:
I did it this way so that I don't have to add `F extends ContentFile<F>` to these static methods and reuse the bound already defined in the class. I could use `?` in the second one but not here.
--
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 merged pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #8432:
URL: https://github.com/apache/iceberg/pull/8432
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312400631
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do you need to consider the condition?
```
!partitionSet.isEmpty()
```
How about adding another method `hasPartitionSet`?
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312411625
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
partitionSet.isEmpty()
```
If the partitionSet is empty, we seems to filter all entries. Maybe we can return empty entries directly when the partitionSet is empty. I'm not sure whether we have guaranteed the partitionSet is not empty.
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1313632181
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -266,12 +280,16 @@ private CloseableIterable<ManifestEntry<F>> open(Schema projection) {
}
CloseableIterable<ManifestEntry<F>> liveEntries() {
- return CloseableIterable.filter(
- content == FileType.DATA_FILES
- ? scanMetrics.skippedDataFiles()
- : scanMetrics.skippedDeleteFiles(),
- entries(),
- entry -> entry != null && entry.status() != ManifestEntry.Status.DELETED);
+ return entries(true /* only live entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> filterLiveEntries(
Review Comment:
I did it this way so that I don't have to add `F extends ContentFile<F>` to these static methods and reuse the bound already defined in the class instead. I could use `?` in the second one but not here.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312400631
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do we need to consider the condition?
```
!partitionSet.isEmpty()
```
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310700313
##########
core/src/test/java/org/apache/iceberg/TestScanPlanningAndReporting.java:
##########
@@ -215,19 +215,24 @@ public void scanningWithSkippedDataFiles() throws IOException {
Table table =
TestTables.create(
tableDir, tableName, SCHEMA, SPEC, SortOrder.unsorted(), formatVersion, reporter);
- table.newAppend().appendFile(FILE_A).appendFile(FILE_D).commit();
- table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).appendFile(FILE_D).commit();
Review Comment:
I need to add `FILE_B` here so that we have a manifest with one skipped data file as the original manifest will be rewritten with `FILE_A` as deleted during the overwrite below.
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1310700313
##########
core/src/test/java/org/apache/iceberg/TestScanPlanningAndReporting.java:
##########
@@ -215,19 +215,24 @@ public void scanningWithSkippedDataFiles() throws IOException {
Table table =
TestTables.create(
tableDir, tableName, SCHEMA, SPEC, SortOrder.unsorted(), formatVersion, reporter);
- table.newAppend().appendFile(FILE_A).appendFile(FILE_D).commit();
- table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).appendFile(FILE_D).commit();
Review Comment:
I need to add `FILE_B` here so that we have a manifest with one skipped data file as the original manifest will be rewritten with `FILE_A` as deleted.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1313682786
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Thanks.
--
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] jerqi commented on a diff in pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1312400631
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
Do you need to consider the condition?
```
!partitionSet.isEmpty()
```
--
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 pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#issuecomment-1699691587
cc @rdblue @nastra
--
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 #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#discussion_r1313562220
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -203,32 +203,46 @@ ManifestReader<F> scanMetrics(ScanMetrics newScanMetrics) {
}
CloseableIterable<ManifestEntry<F>> entries() {
- if ((rowFilter != null && rowFilter != Expressions.alwaysTrue())
- || (partFilter != null && partFilter != Expressions.alwaysTrue())
- || (partitionSet != null)) {
+ return entries(false /* all entries */);
+ }
+
+ private CloseableIterable<ManifestEntry<F>> entries(boolean onlyLive) {
+ if (hasRowFilter() || hasPartitionFilter() || partitionSet != null) {
Review Comment:
I believe an empty `partitionSet` indicates there is no match and acts similar to `alwaysFalse`. We could add a branch for cases when the filters are always false or the matching partition set is empty, but we would have to analyze the usages first to make sure those are possible scenarios.
--
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 pull request #8432: Core: Fix skipped file counts in ManifestReader with deleted entries
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8432:
URL: https://github.com/apache/iceberg/pull/8432#issuecomment-1703505218
Thanks, @rdblue @jerqi!
--
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