wgtmac commented on code in PR #559:
URL: https://github.com/apache/iceberg-cpp/pull/559#discussion_r2896609561
##########
src/iceberg/table_scan.cc:
##########
@@ -355,22 +421,32 @@ TableScanBuilder::ResolveSnapshotSchema() {
return snapshot_schema_;
}
-bool TableScanBuilder::IsIncrementalScan() const {
- return context_.from_snapshot_id.has_value() ||
context_.to_snapshot_id.has_value();
-}
-
-Result<std::unique_ptr<TableScan>> TableScanBuilder::Build() {
+template <>
+Result<std::unique_ptr<DataTableScan>>
TableScanBuilder<DataTableScan>::Build() {
ICEBERG_RETURN_UNEXPECTED(CheckErrors());
ICEBERG_RETURN_UNEXPECTED(context_.Validate());
- if (IsIncrementalScan()) {
- return NotImplemented("Incremental scan is not yet implemented");
- }
-
ICEBERG_ASSIGN_OR_RAISE(auto schema, ResolveSnapshotSchema());
return DataTableScan::Make(metadata_, schema.get(), io_,
std::move(context_));
}
+template <>
+Result<std::unique_ptr<IncrementalAppendScan>>
+TableScanBuilder<IncrementalAppendScan>::Build() {
+ return NotImplemented("IncrementalAppendScanBuilder is not implemented");
+}
+
+template <>
+Result<std::unique_ptr<IncrementalChangelogScan>>
+TableScanBuilder<IncrementalChangelogScan>::Build() {
+ return NotImplemented("IncrementalChangelogScanBuilder is not implemented");
+}
Review Comment:
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:
```cpp
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_));
}
```
##########
src/iceberg/table_scan.cc:
##########
@@ -210,39 +210,73 @@ Result<ArrowArrayStream> FileScanTask::ToArrow(
return MakeArrowArrayStream(std::move(reader));
}
-Result<std::unique_ptr<TableScanBuilder>> TableScanBuilder::Make(
- std::shared_ptr<TableMetadata> metadata, std::shared_ptr<FileIO> io) {
+// Template specialization for DataTableScan (default)
+template <>
+Result<std::unique_ptr<TableScanBuilder<DataTableScan>>>
+TableScanBuilder<DataTableScan>::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>(
- new TableScanBuilder(std::move(metadata), std::move(io)));
+ return std::unique_ptr<TableScanBuilder<DataTableScan>>(
+ new TableScanBuilder<DataTableScan>(std::move(metadata), std::move(io)));
}
-TableScanBuilder::TableScanBuilder(std::shared_ptr<TableMetadata>
table_metadata,
- std::shared_ptr<FileIO> file_io)
+// Template specialization for IncrementalAppendScan
+template <>
+Result<std::unique_ptr<TableScanBuilder<IncrementalAppendScan>>>
+TableScanBuilder<IncrementalAppendScan>::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<IncrementalAppendScan>>(
+ new TableScanBuilder<IncrementalAppendScan>(std::move(metadata),
std::move(io)));
+}
+
+// Template specialization for IncrementalChangelogScan
+template <>
+Result<std::unique_ptr<TableScanBuilder<IncrementalChangelogScan>>>
+TableScanBuilder<IncrementalChangelogScan>::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<IncrementalChangelogScan>>(
Review Comment:
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:
```cpp
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)));
}
```
--
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]
