address feedback

Author: wgtmac

src/iceberg/table_scan.cc

@@ -156,21 +156,15 @@ Result<ArrowArrayStream> MakeArrowArrayStream(std::unique_ptr<Reader> reader) {
 namespace internal {
 
 Status TableScanContext::Validate() const {
-  if (!columns_to_keep_stats.empty() && !return_column_stats) {
-    return InvalidArgument(
-        "Cannot select columns to keep stats when column stats are not returned");
-  }
-  if (projected_schema != nullptr && !selected_columns.empty()) {
-    return InvalidArgument(
-        "Cannot set projection schema and selected columns at the same time");
-  }
-  if (snapshot_id.has_value() &&
-      (from_snapshot_id.has_value() || to_snapshot_id.has_value())) {
-    return InvalidArgument("Cannot mix snapshot scan and incremental scan");
-  }
-  if (min_rows_requested.has_value() && min_rows_requested.value() < 0) {
-    return InvalidArgument("Min rows requested cannot be negative");
-  }
+  ICEBERG_CHECK(columns_to_keep_stats.empty() || return_column_stats,
+                "Cannot select columns to keep stats when column stats are not returned");
+  ICEBERG_CHECK(projected_schema == nullptr || selected_columns.empty(),
+                "Cannot set projection schema and selected columns at the same time");
+  ICEBERG_CHECK(!snapshot_id.has_value() ||
+                    (!from_snapshot_id.has_value() && !to_snapshot_id.has_value()),
+                "Cannot mix snapshot scan and incremental scan");
+  ICEBERG_CHECK(!min_rows_requested.has_value() || min_rows_requested.value() >= 0,
+                "Min rows requested cannot be negative");
   return {};
 }
 
@@ -320,23 +314,13 @@ TableScanBuilder& TableScanBuilder::AsOfTime(int64_t timestamp_millis) {
   return UseSnapshot(snapshot_id);
 }
 
-TableScanBuilder& TableScanBuilder::FromSnapshotInclusive(
-    [[maybe_unused]] int64_t from_snapshot_id) {
-  return AddError(NotImplemented("Incremental scan is not implemented"));
-}
-
-TableScanBuilder& TableScanBuilder::FromSnapshotInclusive(
-    [[maybe_unused]] const std::string& ref) {
-  return AddError(NotImplemented("Incremental scan is not implemented"));
-}
-
-TableScanBuilder& TableScanBuilder::FromSnapshotExclusive(
-    [[maybe_unused]] int64_t from_snapshot_id) {
+TableScanBuilder& TableScanBuilder::FromSnapshot(
+    [[maybe_unused]] int64_t from_snapshot_id, [[maybe_unused]] bool inclusive) {
   return AddError(NotImplemented("Incremental scan is not implemented"));
 }
 
-TableScanBuilder& TableScanBuilder::FromSnapshotExclusive(
-    [[maybe_unused]] const std::string& ref) {
+TableScanBuilder& TableScanBuilder::FromSnapshot([[maybe_unused]] const std::string& ref,
+                                                 [[maybe_unused]] bool inclusive) {
   return AddError(NotImplemented("Incremental scan is not implemented"));
 }
 

src/iceberg/table_scan.h

@@ -204,49 +204,27 @@ class ICEBERG_EXPORT TableScanBuilder : public ErrorCollector {
   /// travel is attempted on a tag
   TableScanBuilder& AsOfTime(int64_t timestamp_millis);
 
-  /// \brief Instructs this scan to look for changes starting from a particular snapshot
-  /// (inclusive).
-  ///
-  /// If the start snapshot is not configured, it defaults to the oldest ancestor of the
-  /// end snapshot (inclusive).
-  ///
-  /// \param from_snapshot_id the start snapshot ID (inclusive)
-  /// \note InvalidArgument will be returned if the start snapshot is not an ancestor of
-  /// the end snapshot
-  TableScanBuilder& FromSnapshotInclusive(int64_t from_snapshot_id);
-
-  /// \brief Instructs this scan to look for changes starting from a particular snapshot
-  /// (inclusive).
-  ///
-  /// If the start snapshot is not configured, it defaults to the oldest ancestor of the
-  /// end snapshot (inclusive).
-  ///
-  /// \param ref the start ref name that points to a particular snapshot ID (inclusive)
-  /// \note InvalidArgument will be returned if the start snapshot is not an ancestor of
-  /// the end snapshot
-  TableScanBuilder& FromSnapshotInclusive(const std::string& ref);
-
-  /// \brief Instructs this scan to look for changes starting from a particular snapshot
-  /// (exclusive).
+  /// \brief Instructs this scan to look for changes starting from a particular snapshot.
   ///
   /// If the start snapshot is not configured, it defaults to the oldest ancestor of the
   /// end snapshot (inclusive).
   ///
-  /// \param from_snapshot_id the start snapshot ID (exclusive)
+  /// \param from_snapshot_id the start snapshot ID
+  /// \param inclusive whether the start snapshot is inclusive, default is false
   /// \note InvalidArgument will be returned if the start snapshot is not an ancestor of
   /// the end snapshot
-  TableScanBuilder& FromSnapshotExclusive(int64_t from_snapshot_id);
+  TableScanBuilder& FromSnapshot(int64_t from_snapshot_id, bool inclusive = false);
 
-  /// \brief Instructs this scan to look for changes starting from a particular snapshot
-  /// (exclusive).
+  /// \brief Instructs this scan to look for changes starting from a particular snapshot.
   ///
   /// If the start snapshot is not configured, it defaults to the oldest ancestor of the
   /// end snapshot (inclusive).
   ///
-  /// \param ref the start ref name that points to a particular snapshot ID (exclusive)
+  /// \param ref the start ref name that points to a particular snapshot ID
+  /// \param inclusive whether the start snapshot is inclusive, default is false
   /// \note InvalidArgument will be returned if the start snapshot is not an ancestor of
   /// the end snapshot
-  TableScanBuilder& FromSnapshotExclusive(const std::string& ref);
+  TableScanBuilder& FromSnapshot(const std::string& ref, bool inclusive = false);
 
   /// \brief Instructs this scan to look for changes up to a particular snapshot
   /// (inclusive).