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 2021/04/25 12:20:01 UTC
[GitHub] [iceberg] coolderli opened a new pull request #2510: Core: Add content to ManifestTable Schema.
coolderli opened a new pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510
We need to add the content field to the schema of AllManifestsTable, so that we can get the type of manifest for expiration processing.
--
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] coolderli commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826460930
@rdblue @openinx could you help me review it? 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.
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] coolderli commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r621011474
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -53,8 +53,9 @@
Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
- )))
- );
+ ))),
+ Types.NestedField.optional(14, "content", Types.StringType.get())
+ );
Review comment:
done
--
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] coolderli commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r619952644
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
OK, I will fix it. 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.
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] coolderli commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-828108799
@jackye1995 Thank you for reviewing the code.
--
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] jackye1995 commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r620837117
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
Prefer to add it as the last entry so that ID of other fields are not modified for backwards compatibility.
--
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] openinx commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r619939799
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
The correct way to add a new field in this manifest table is : add the `content` field with an auto-increment field id (which should be 14 in this case) at the tail of those nested field list. We cannot change the existing field ids in this schema because them are mapping to the underlying rows in generated avro files.
--
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] coolderli commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826789046
Sorry, I didn't make it clear. This patch is actually a part of #2518.
--
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] coolderli commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826479272
@openinx I want to support the expiring snapshot of the V2 table
After we rewrite the data file and the delete file in #2303, we can delete the corresponding data files and delete files through manifest when expiring the snapshot.
--
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] jackye1995 commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r620906690
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -53,8 +53,9 @@
Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
- )))
- );
+ ))),
+ Types.NestedField.optional(14, "content", Types.StringType.get())
+ );
Review comment:
nit: should not change space for unmodified lines.
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -41,8 +41,9 @@
Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
- )))
- );
+ ))),
+ Types.NestedField.required(14, "content", Types.StringType.get())
+ );
Review comment:
nit: should not change space for unmodified lines.
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -94,8 +95,9 @@ protected DataTask task(TableScan scan) {
manifest.addedFilesCount(),
manifest.existingFilesCount(),
manifest.deletedFilesCount(),
- partitionSummariesToRows(spec, manifest.partitions())
- );
+ partitionSummariesToRows(spec, manifest.partitions()),
+ manifest.content().name()
+ );
Review comment:
nit: should not change space for unmodified lines.
--
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] coolderli commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r619952644
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
OK, I will fix it. 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.
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] coolderli commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826460930
--
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] openinx commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826483934
So why need to add the extra nested field in table schema ? I still not get the 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.
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] openinx commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r619939799
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
The correct way to add a new field in this manifest table is : add the `content` field with an auto-increment field id (which should be 14 in this case) at the tail of those nested field list. We cannot change the existing field ids in this schema because them are mapping to the underlying rows in generated avro files.
--
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] jackye1995 commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r620837466
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
I would prefer it to be a String field, showing content 0 or 1 does not make too much sense from user perspective, comparing to showing `DATA` or `DELETES`
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -44,15 +44,16 @@
Types.NestedField.required(1, "path", Types.StringType.get()),
Types.NestedField.required(2, "length", Types.LongType.get()),
Types.NestedField.optional(3, "partition_spec_id", Types.IntegerType.get()),
- Types.NestedField.optional(4, "added_snapshot_id", Types.LongType.get()),
- Types.NestedField.optional(5, "added_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(6, "existing_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(7, "deleted_data_files_count", Types.IntegerType.get()),
- Types.NestedField.optional(8, "partition_summaries", Types.ListType.ofRequired(9, Types.StructType.of(
- Types.NestedField.required(10, "contains_null", Types.BooleanType.get()),
- Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
- Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
- Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+ Types.NestedField.optional(4, "content", Types.IntegerType.get()),
Review comment:
Prefer to add it as the last entry so that ID of other fields are not notified for backwards compatibility.
--
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 #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r635666462
##########
File path: core/src/test/java/org/apache/iceberg/TestManifestReader.java
##########
@@ -145,4 +145,67 @@ public void testDeleteFilePositions() throws IOException {
}
}
}
+
+ @Test
+ public void testReadDataManifestTable() throws IOException {
Review comment:
I don't think that these tests should be in this class. This suite tests the manifest reader, not the manifest metadata tables. Why not put these tests in `TestIcebergSourceTablesBase`?
--
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] openinx commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-826483934
So why need to add the extra nested field in table schema ? I still not get the 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.
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] jackye1995 commented on pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#issuecomment-827785132
Sorry for the back and forth, now I think about it, the files table has this in schema:
```java
optional(134, "content", IntegerType.get(), "Contents of the file: 0=data, 1=position deletes, 2=equality deletes");
```
So it would make more sense to use int also for manifest, but we should add a doc similar to the files table like:
```java
optional(14, "content", IntegerType.get(), "Contents of the manifest: 0=data, 1=deletes");
```
so that people know what the values mean.
Apart from that, I don't have any further comments, thanks for the work.
--
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] coolderli commented on a change in pull request #2510: Core: Add content field to ManifestTable Schema.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2510:
URL: https://github.com/apache/iceberg/pull/2510#discussion_r667630988
##########
File path: core/src/test/java/org/apache/iceberg/TestManifestReader.java
##########
@@ -145,4 +145,67 @@ public void testDeleteFilePositions() throws IOException {
}
}
}
+
+ @Test
+ public void testReadDataManifestTable() throws IOException {
Review comment:
I think this [test ](https://github.com/apache/iceberg/pull/2510/files#diff-9bb9188f62743b673504bdf41abf06d378ccb49443fd9828b0b65e88cdde7022R720) has covered the use cases.
--
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