rdblue commented on a change in pull request #2617:
URL: https://github.com/apache/iceberg/pull/2617#discussion_r655764931
##########
File path: core/src/main/java/org/apache/iceberg/StaticTableScan.java
##########
@@ -25,16 +25,33 @@
class StaticTableScan extends BaseTableScan {
private final Function<StaticTableScan, DataTask> buildTask;
+ // Metadata table name that the buildTask that this StaticTableScan will
return data for.
+ private final String scannedTableName;
- StaticTableScan(TableOperations ops, Table table, Schema schema,
Function<StaticTableScan, DataTask> buildTask) {
+ StaticTableScan(TableOperations ops, Table table, Schema schema, String
scannedTableName,
+ Function<StaticTableScan, DataTask> buildTask) {
super(ops, table, schema);
this.buildTask = buildTask;
+ this.scannedTableName = scannedTableName;
}
- private StaticTableScan(TableOperations ops, Table table, Schema schema,
+ private StaticTableScan(TableOperations ops, Table table, Schema schema,
String scannedTableName,
Function<StaticTableScan, DataTask> buildTask,
TableScanContext context) {
super(ops, table, schema, context);
this.buildTask = buildTask;
+ this.scannedTableName = scannedTableName;
+ }
+
+ @Override
+ public TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ throw new UnsupportedOperationException(
+ String.format("Incremental scan is not supported for metadata table
%s", scannedTableName));
+ }
+
+ @Override
+ public TableScan appendsAfter(long fromSnapshotId) {
+ throw new UnsupportedOperationException(
+ String.format("Incremental scan is not supported for metadata table
%s", scannedTableName));
Review comment:
Coming back to this now. I think that we probably don't want to make the
`entries` and `files` tables use `StaticTableScan` because `StaticTableScan`
assumes that there will be just one task created. The `entries` table is
designed to actually create lots of tasks that can run in parallel and read
when `rows` is called, in contrast to the static tables that read all necessary
files on the driver and create a task that contains the serialized rows.
##########
File path: core/src/main/java/org/apache/iceberg/StaticTableScan.java
##########
@@ -26,7 +26,8 @@
class StaticTableScan extends BaseTableScan {
private final Function<StaticTableScan, DataTask> buildTask;
- StaticTableScan(TableOperations ops, Table table, Schema schema,
Function<StaticTableScan, DataTask> buildTask) {
+ StaticTableScan(TableOperations ops, Table table, Schema schema,
+ Function<StaticTableScan, DataTask> buildTask) {
Review comment:
Looks like this didn't need to change.
##########
File path: core/src/main/java/org/apache/iceberg/StaticTableScan.java
##########
@@ -37,6 +38,28 @@ private StaticTableScan(TableOperations ops, Table table,
Schema schema,
this.buildTask = buildTask;
}
+ /**
+ * Type of scan being performed by the buildTask, such as {@link
MetadataTableType#HISTORY} when scanning
+ * a table's {@link org.apache.iceberg.HistoryTable}.
+ * <p>
+ * Used for logging and error messages.
+ */
+ protected String tableType() {
+ return "static";
Review comment:
Rather than overriding this several places and using a generic string
here to avoid making this class abstract, I suggest passing the
`MetadataTableType` in through the constructor so this just returns
`tableType.name()`.
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java
##########
@@ -60,6 +60,19 @@ public void testCannotBeDeletedFrom() {
() -> staticTable.newDelete().deleteFile(FILE_A).commit());
}
+ @Test
+ public void testCannotDoIncrementalScanOnMetadataTable() {
+ table.newAppend().appendFile(FILE_A).commit();
+
+ for (MetadataTableType type : MetadataTableType.values()) {
+ Table staticTable = getStaticTable(type);
+ AssertHelpers.assertThrows("Static tables do not currently support
incremental scans",
Review comment:
I think that a better place for this test is now
`TestMetadataTableScans` since it supports all metadata tables. Also, the
context message still says "Static tables . . .".
##########
File path: core/src/main/java/org/apache/iceberg/StaticTableScan.java
##########
@@ -37,6 +38,28 @@ private StaticTableScan(TableOperations ops, Table table,
Schema schema,
this.buildTask = buildTask;
}
+ /**
+ * Type of scan being performed by the buildTask, such as {@link
MetadataTableType#HISTORY} when scanning
+ * a table's {@link org.apache.iceberg.HistoryTable}.
+ * <p>
+ * Used for logging and error messages.
+ */
+ protected String tableType() {
+ return "static";
+ }
+
+ @Override
+ public TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ throw new UnsupportedOperationException(
+ String.format("Incremental scan is not supported for %s scan of table
%s", tableType(), table().name()));
+ }
+
+ @Override
+ public TableScan appendsAfter(long fromSnapshotId) {
+ throw new UnsupportedOperationException(
+ String.format("Incremental scan is not supported for %s scan of table
%s", tableType(), table().name()));
Review comment:
This error message makes it sound like it is a combination of the table
and the metadata table type, but no metadata tables support incremental scan
(yet?).
I think this should state that the metadata table does not support
incremental scan: `"Cannot incrementally scan metadata table: %s"`
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
