rdblue commented on a change in pull request #1115:
URL: https://github.com/apache/iceberg/pull/1115#discussion_r441176413
##########
File path: core/src/main/java/org/apache/iceberg/IncrementalDataTableScan.java
##########
@@ -19,55 +19,43 @@
package org.apache.iceberg;
-import java.util.Collection;
import java.util.List;
import java.util.Set;
-import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.FluentIterable;
-import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.util.SnapshotUtil;
import org.apache.iceberg.util.ThreadPools;
class IncrementalDataTableScan extends DataTableScan {
- private long fromSnapshotId;
- private long toSnapshotId;
-
- IncrementalDataTableScan(TableOperations ops, Table table, Schema schema,
Expression rowFilter,
- boolean ignoreResiduals, boolean caseSensitive,
boolean colStats,
- Collection<String> selectedColumns,
ImmutableMap<String, String> options,
- long fromSnapshotId, long toSnapshotId) {
- super(ops, table, null, schema, rowFilter, ignoreResiduals, caseSensitive,
colStats, selectedColumns, options);
- validateSnapshotIds(table, fromSnapshotId, toSnapshotId);
- this.fromSnapshotId = fromSnapshotId;
- this.toSnapshotId = toSnapshotId;
+
+ IncrementalDataTableScan(TableOperations ops, Table table, Schema schema,
TableScanContext context) {
+ super(ops, table, schema, context.useSnapshotId(null));
Review comment:
The guarantee that the snapshot ID is currently null is the check done
in the refinement methods that create an incremental scan. I think those
validations should be done in the context because the context should be
responsible for knowing what is a valid scan. If a table doesn't support
incremental scanning, then it can have additional validations, but anything
common should be in the context.
That would allow you to not set the snapshot to null here. We may also want
to get rid of `IncrementalDataTableScan`, though probably not in this PR. I
think the purpose of it was to minimize changes to `BaseTableScan`. But this
refactor makes that much less necessary.
----------------------------------------------------------------
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]
