Core: Address Anton's comments from the PR 4744 (incremental append scan impl)#4886
Conversation
|
For the boolean flag comment, do you think a builder class for For the helper methods comment, I feel we are adding a bunch of methods without making the code much simpler. For the schema evolution comment, I think it is outside the scope of streaming source. |
| return TableScanUtil.planTasks(splitFiles, targetSplitSize(), splitLookback(), splitOpenFileCost()); | ||
| } | ||
|
|
||
| private long toSnapshotIdExclusive() { |
There was a problem hiding this comment.
Isn't toSnapshotId always inclusive, not exclusive?
There was a problem hiding this comment.
good catch. I meant inclusive. somehow typed exclusive
| // by finding the oldest ancestor of end snapshot. | ||
| Long fromSnapshotIdExclusive = context().fromSnapshotId(); | ||
| if (context().fromSnapshotId() != null) { | ||
| Long fromSnapshotIdExclusive = fromSnapshotId; |
There was a problem hiding this comment.
If fromSnapshotId may be inclusive, then it should never be assigned to fromSnapshotIdExclusive without first checking. Otherwise, there is temporarily a value for fromSnapshotIdExclusive that is incorrect. If you have a guarantee in the variable name, you should never violate that guarantee because someone might not know about it later.
There was a problem hiding this comment.
good point. will update
| protected abstract T newRefinedScan( | ||
| TableOperations newOps, Table newTable, Schema newSchema, TableScanContext newContext); | ||
| protected abstract T newRefinedScan(TableOperations newOps, Table newTable, Schema newSchema, | ||
| TableScanContext newContext); |
There was a problem hiding this comment.
I don't think these reformatting changes are worth it, since the alternative is already in master.
| * @return null if there is no current snapshot in the table, else the oldest Snapshot. | ||
| */ | ||
| public static Snapshot oldestAncestor(long snapshotId, Function<Long, Snapshot> lookup) { | ||
| public static Snapshot oldestAncestorOf(long snapshotId, Function<Long, Snapshot> lookup) { |
There was a problem hiding this comment.
Didn't this exist before the other commit? If this has been released, I doubt it is a good idea to rename it just to add "Of".
There was a problem hiding this comment.
this was added by the other PR #4744 (incremental append scan impl) that just merged. It is not released and should be safe to rename
There was a problem hiding this comment.
It was just added and hasn't been released. All other methods in this class follow this name pattern.
stevenzwu
changed the title
aokolnychyi
left a comment
aokolnychyi
left a comment
There was a problem hiding this comment.
Thanks, @stevenzwu! I had one question in TableScanContext but looks good to me otherwise.
| TableScanContext fromSnapshotIdExclusive(long id) { | ||
| return new TableScanContext(snapshotId, rowFilter, ignoreResiduals, | ||
| caseSensitive, colStats, projectedSchema, selectedColumns, options, id, toSnapshotId, | ||
| planExecutor, fromSnapshotInclusive); |
There was a problem hiding this comment.
nit: I think we should probably pass false explicitly instead of using fromSnapshotInclusive.
There was a problem hiding this comment.
agree. explicit false is more clear.
| * @return null if there is no current snapshot in the table, else the oldest Snapshot. | ||
| */ | ||
| public static Snapshot oldestAncestor(long snapshotId, Function<Long, Snapshot> lookup) { | ||
| public static Snapshot oldestAncestorOf(long snapshotId, Function<Long, Snapshot> lookup) { |
There was a problem hiding this comment.
It was just added and hasn't been released. All other methods in this class follow this name pattern.
|
Thanks, @stevenzwu! |
3 participants
No description provided.