szehon-ho commented on code in PR #4382:
URL: https://github.com/apache/iceberg/pull/4382#discussion_r842136719
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTableScan.java:
##########
@@ -31,11 +37,30 @@ protected BaseMetadataTableScan(TableOperations ops, Table
table, Schema schema,
super(ops, table, schema, context);
}
+ /**
+ * @return if metadata table scan is for all snapshots, ie 'all_x' metadata
tables
+ */
+ protected boolean allScan() {
Review Comment:
@aokolnychyi yea i had that version in the previous iteration, having a
common method for the BaseAllMetadataTableScan and AllDataFileTableScan (which
no longer inherits from it), like:
```
BaseMetadataTableScan.planFilesForAllSnapshots();
```
and then having:
```
@Override
BaseAllMetadataTableScan.planFiles() {
super.planFilesForAllSnapshots();
}
@Override
AllDataFileTableScan.planFiles() {
super.planFilesForAllSnapshots();
}
```
Is that better than this? I can revert back to that version.
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTableScan.java:
##########
@@ -31,11 +37,30 @@ protected BaseMetadataTableScan(TableOperations ops, Table
table, Schema schema,
super(ops, table, schema, context);
}
+ /**
+ * @return if metadata table scan is for all snapshots, ie 'all_x' metadata
tables
+ */
+ protected boolean allScan() {
Review Comment:
The reason I changed to this approach , it's slightly more clear in forcing
implementators to have to override this flag, rather than have to find this
method just by reading the code , but it's not a big deal either way as it's
fairly internal.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
