feat: Add incremental scan API with IncrementalAppendScan and Increme…

[WZhuo]

No description provided.

[wgtmac]

Review Report: PR #559

(Generated by gemini-cli)

📄 Files: src/iceberg/table_scan.h & src/iceberg/table_scan.cc

Java Counterpart: TableScan.java / IncrementalScan.java

• Parity Check & API Design: ❌

  • CRITICAL API FLAW: The PR makes BaseIncrementalScanBuilder inherit from TableScanBuilder. However, TableScanBuilder's methods (like Filter(), Project(), Select()) return TableScanBuilder&.
  • This breaks the fluent builder pattern. If a user tries to chain methods:

    table->NewIncrementalAppendScan().value()
         ->Filter(expr)
         .FromSnapshot(1); // ERROR: TableScanBuilder has no FromSnapshot method

    It will fail to compile because Filter() returns TableScanBuilder&.
  • Furthermore, calling Build() after Filter() will invoke TableScanBuilder::Build(), which returns a DataTableScan instead of the expected IncrementalAppendScan.
  • Action: Refactor the builders to use the Curiously Recurring Template Pattern (CRTP) for a common base builder (e.g., template <typename Derived> class ScanBuilderBase), so that inherited methods return the correct derived type. Alternatively, override the methods in the derived builders to cast and return *this.
  • Good catch: Removing UseBranch from TableScanBuilder and moving it to BaseIncrementalScanBuilder perfectly matches Java's parity, where useBranch only exists in IncrementalScan.

• Style & Comments: ⚠️

  • Java uses fromSnapshotInclusive(long) and fromSnapshotExclusive(long). The PR uses FromSnapshot(int64_t, bool inclusive = false). While acceptable in C++, boolean arguments can cause "boolean blindness" at the call site (e.g., FromSnapshot(100, true)). Consider using an enum (e.g., enum class SnapshotBoundary { kInclusive, kExclusive }) or mirroring Java's explicit method names for better readability.

• Logic Check: ❌

  • In table_scan.cc, the Build() implementation is hardcoded to return the same error message regardless of the ScanType:

    template <typename ScanType>
    Result<std::unique_ptr<ScanType>> IncrementalScanBuilder<ScanType>::Build() {
      return NotImplemented("IncrementalAppendScanBuilder is not implemented");
    }

    Action: If ScanType is IncrementalChangelogScan, the error message is misleading. Update the error message to be generic (e.g., "Incremental scan builder is not implemented") or use typeid or template specialization to provide an accurate message.

• Design & Conciseness: ✅

  • The separation of DataTableScan and the new incremental scan classes is logically sound and structurally aligned with the Iceberg specification.

• Test Quality: ❌

  • No tests were added for the new incremental builders. While they currently return NotImplemented, there should be basic instantiation tests to ensure the builders can be created from Table::NewIncremental...().
  • Action: Add a test case that specifically chains a common method (like Filter) with an incremental method (like FromSnapshot) to ensure the fluent API compiles and works correctly once the CRTP/inheritance issue is fixed.

---

📄 Files: src/iceberg/table.h & src/iceberg/table.cc

Java Counterpart: Table.java

• Parity Check: ✅ The new methods NewIncrementalAppendScan() and NewIncrementalChangelogScan() match Java's Table interface. Returning a builder instead of the scan itself aligns with the existing C++ paradigm (NewScan() returning TableScanBuilder).
• Style & Comments: ✅ Clean and consistent with the rest of the file.
• Logic Check: ✅
• Design & Conciseness: ✅
• Test Quality: ⚠️ No tests were added to verify that Table::NewIncrementalAppendScan() successfully returns a builder.

---

Summary & Recommendation

• Request Changes
• The PR has a critical API design flaw regarding the C++ Builder pattern inheritance. Because TableScanBuilder methods return TableScanBuilder&, method chaining in IncrementalScanBuilder is broken. The builder hierarchy needs to be refactored (likely using CRTP) to support fluent chaining correctly. Please also address the hardcoded error message in the template builder and add basic tests to catch compile-time API issues.

[gty404]

UseBranch should also be used in non-incremental scan scenarios ？

[WZhuo]

branch is only used in the IncrementalScan, for validating the to_snapshot_id is on the specified branch.

[wgtmac]

Technically snapshot read of a table scan should support branch. So I still think it should be kept even if the Java api does not implement this.

[WZhuo]

The UseRef api supports both Tag and Branch for a TableScan.

[wgtmac]

I know that the intention of this class hierarchy design is to reuse code. However, this duplicates all apis of the builder so it does not seem worth the effort.

[WZhuo]

Any good suggestions?

[wgtmac]

Use a template scan builder class for all and then use std::enable_if to enable functions of incremental scan builder?

[wgtmac]

This review was generated by gemini-cli.

The overall API design in this PR is excellent. You've done a great job aligning with the Java API parity while making smart use of Modern C++ features. In particular:

• Using C++20 requires concepts (requires IsIncrementalScan<ScanType>) to restrict methods at compile-time is a much safer and cleaner approach than Java's runtime UnsupportedOperationException.
• Consolidating Java's fromSnapshotInclusive / fromSnapshotExclusive into a single FromSnapshot with a default inclusive boolean parameter is a great C++ idiom that reduces the API surface.

However, there is noticeable code duplication in the template implementations in .cc that should be cleaned up. Please see the inline comments for details.

[wgtmac]

There is noticeable code duplication for the Make method specializations (DataTableScan, IncrementalAppendScan, IncrementalChangelogScan). They are completely identical.

You can remove these three explicit specializations and provide a single generic implementation that relies on the template class ... instantiations at the end of the file:

template <typename ScanType>
Result<std::unique_ptr<TableScanBuilder<ScanType>>>
TableScanBuilder<ScanType>::Make(std::shared_ptr<TableMetadata> metadata,
                                 std::shared_ptr<FileIO> io) {
  ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null");
  ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null");
  return std::unique_ptr<TableScanBuilder<ScanType>>(
      new TableScanBuilder<ScanType>(std::move(metadata), std::move(io)));
}

[wgtmac]

Similar to the Make method, these Build() explicit specializations for IncrementalAppendScan and IncrementalChangelogScan are redundant and can be generalized.

If you consolidate this to a single generic Build() implementation, it would just call ScanType::Make(...), and since IncrementalAppendScan::Make already returns NotImplemented, the error would bubble up naturally. This will help reduce boilerplate code:

template <typename ScanType>
Result<std::unique_ptr<ScanType>> TableScanBuilder<ScanType>::Build() {
  ICEBERG_RETURN_UNEXPECTED(CheckErrors());
  ICEBERG_RETURN_UNEXPECTED(context_.Validate());

  ICEBERG_ASSIGN_OR_RAISE(auto schema, ResolveSnapshotSchema());
  return ScanType::Make(metadata_, schema.get(), io_, std::move(context_));
}