You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2024/07/23 21:16:42 UTC
[PR] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
szehon-ho opened a new pull request, #10759:
URL: https://github.com/apache/iceberg/pull/10759
Spark queries of Metadata Tables will often try to check row count estimates repeatedly during planning phase, like picking a join strategy.
By default, this involves an extra call to read the manifest file header, as the default logic on the BaseContentScanTask is as follows:
```
static long estimateRowsCount(long length, ContentFile<?> file) {
long[] splitOffsets = splitOffsets(file);
long splitOffset = splitOffsets != null ? splitOffsets[0] : 0L;
double scannedFileFraction = ((double) length) / (file.fileSizeInBytes() - splitOffset);
return (long) (scannedFileFraction * file.recordCount());
}
```
However, we actually have this information in the metadata and can just return it directly without reading the file header.
This can save many calls on the Spark driver for planning (one call per manifest file, so potentially many thousands, in serial manner).
This is useful as many Spark maintenance procedures involve querying these tables ( expireSnapshots, removeOrphanFiles)
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#discussion_r1690185573
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1753,4 +1753,36 @@ public void testPositionDeletesManyColumns() {
assertThat(scanTasks.get(0).file().path().toString()).isEqualTo("/path/to/delete1.parquet");
assertThat(scanTasks.get(1).file().path().toString()).isEqualTo("/path/to/delete2.parquet");
}
+
+ @TestTemplate
+ public void testFilesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new DataFilesTable(table));
+ assertEstimatedSize(new AllDataFilesTable(table));
+ assertEstimatedSize(new AllFilesTable(table));
+
+ if (formatVersion == 2) {
+ assertEstimatedSize(new DeleteFilesTable(table));
+ assertEstimatedSize(new AllDeleteFilesTable(table));
+ }
+ }
+
+ @TestTemplate
+ public void testEntriesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new ManifestEntriesTable(table));
+ assertEstimatedSize(new AllEntriesTable(table));
+ }
+
+ private void assertEstimatedSize(Table metadataTable) throws Exception {
+ TableScan scan = metadataTable.newScan();
+
+ try (CloseableIterable<FileScanTask> tasks = scan.planFiles()) {
+ List<FileScanTask> taskList = Lists.newArrayList(tasks);
+ assertThat(taskList.size()).isGreaterThan(0);
+ taskList.forEach(task -> assertThat(task.estimatedRowsCount()).isEqualTo(4));
Review Comment:
Super nit since I know it's the same table in the tests, but may be worth passing this estimated row count as a parameter to `assertEstimatedSize` just so it's flexible if someone wants to write their own test with a different table or something.
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#discussion_r1688784074
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1753,4 +1753,35 @@ public void testPositionDeletesManyColumns() {
assertThat(scanTasks.get(0).file().path().toString()).isEqualTo("/path/to/delete1.parquet");
assertThat(scanTasks.get(1).file().path().toString()).isEqualTo("/path/to/delete2.parquet");
}
+
+ @TestTemplate
+ public void testFilesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new DataFilesTable(table));
+ assertEstimatedSize(new AllDataFilesTable(table));
+ assertEstimatedSize(new AllFilesTable(table));
+
+ if (formatVersion != 2) {
+ assertEstimatedSize(new DeleteFilesTable(table));
+ assertEstimatedSize(new AllDeleteFilesTable(table));
+ }
Review Comment:
You are right, typo, i think there are no tasks so assert passed. Let me rewrite it.
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#issuecomment-2246857283
cc @amogh-jahagirdar @RussellSpitzer does it make sense? 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
Re: [PR] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #10759:
URL: https://github.com/apache/iceberg/pull/10759
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "lurnagao-dahua (via GitHub)" <gi...@apache.org>.
lurnagao-dahua commented on PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#issuecomment-2252312259
Hi,May I ask why we are not using manifest.addedRowsCount() instead of manifest.addedFilesCount()?
deleted and existing is the same?
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#issuecomment-2253051465
This is for a metadata table and not data table. Hence for manifest-read task, each manifest's row count is the number of files it references (added + deleted + existing)
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#discussion_r1688761399
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1753,4 +1753,35 @@ public void testPositionDeletesManyColumns() {
assertThat(scanTasks.get(0).file().path().toString()).isEqualTo("/path/to/delete1.parquet");
assertThat(scanTasks.get(1).file().path().toString()).isEqualTo("/path/to/delete2.parquet");
}
+
+ @TestTemplate
+ public void testFilesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new DataFilesTable(table));
+ assertEstimatedSize(new AllDataFilesTable(table));
+ assertEstimatedSize(new AllFilesTable(table));
+
+ if (formatVersion != 2) {
+ assertEstimatedSize(new DeleteFilesTable(table));
+ assertEstimatedSize(new AllDeleteFilesTable(table));
+ }
Review Comment:
Maybe misunderstanding something but shouldn't this be `if (formatVersion == 2)` for testing the delete files metadata tables?
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "lurnagao-dahua (via GitHub)" <gi...@apache.org>.
lurnagao-dahua commented on PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#issuecomment-2253706391
> This is for a metadata table and not data table. Hence for manifest-read task, each manifest's row count is the number of files it references (added + deleted + existing)
Got it, thank you for your answer
--
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] Core: Add estimateRowCount for Files and Entries Metadata Tables [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #10759:
URL: https://github.com/apache/iceberg/pull/10759#discussion_r1690227872
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1753,4 +1753,36 @@ public void testPositionDeletesManyColumns() {
assertThat(scanTasks.get(0).file().path().toString()).isEqualTo("/path/to/delete1.parquet");
assertThat(scanTasks.get(1).file().path().toString()).isEqualTo("/path/to/delete2.parquet");
}
+
+ @TestTemplate
+ public void testFilesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new DataFilesTable(table));
+ assertEstimatedSize(new AllDataFilesTable(table));
+ assertEstimatedSize(new AllFilesTable(table));
+
+ if (formatVersion == 2) {
+ assertEstimatedSize(new DeleteFilesTable(table));
+ assertEstimatedSize(new AllDeleteFilesTable(table));
+ }
+ }
+
+ @TestTemplate
+ public void testEntriesTableEstimateSize() throws Exception {
+ preparePartitionedTable(true);
+
+ assertEstimatedSize(new ManifestEntriesTable(table));
+ assertEstimatedSize(new AllEntriesTable(table));
+ }
+
+ private void assertEstimatedSize(Table metadataTable) throws Exception {
+ TableScan scan = metadataTable.newScan();
+
+ try (CloseableIterable<FileScanTask> tasks = scan.planFiles()) {
+ List<FileScanTask> taskList = Lists.newArrayList(tasks);
+ assertThat(taskList.size()).isGreaterThan(0);
+ taskList.forEach(task -> assertThat(task.estimatedRowsCount()).isEqualTo(4));
Review Comment:
Makes sense, ill make the 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