resolve comment to use Result in CoalesceReadRanges

mapleFU · Oct 6, 2023 · f36532d · f36532d
1 parent 9bc42b6
commit f36532d
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 25 deletions.
diff --git a/cpp/src/arrow/io/caching.cc b/cpp/src/arrow/io/caching.cc
@@ -174,8 +174,9 @@ struct ReadRangeCache::Impl {
 
   // Add the given ranges to the cache, coalescing them where possible
   virtual Status Cache(std::vector<ReadRange> ranges) {
-    ranges = internal::CoalesceReadRanges(std::move(ranges), options.hole_size_limit,
-                                          options.range_size_limit);
+    ARROW_ASSIGN_OR_RAISE(
+        ranges, internal::CoalesceReadRanges(std::move(ranges), options.hole_size_limit,
+                                             options.range_size_limit));
     std::vector<RangeCacheEntry> new_entries = MakeCacheEntries(ranges);
     // Add new entries, themselves ordered by offset
     if (entries.size() > 0) {

diff --git a/cpp/src/arrow/io/interfaces.cc b/cpp/src/arrow/io/interfaces.cc
@@ -425,7 +425,7 @@ ThreadPool* GetIOThreadPool() {
 namespace {
 
 struct ReadRangeCombiner {
-  std::vector<ReadRange> Coalesce(std::vector<ReadRange> ranges) {
+  Result<std::vector<ReadRange>> Coalesce(std::vector<ReadRange> ranges) {
     if (ranges.empty()) {
       return ranges;
     }
@@ -454,7 +454,9 @@ struct ReadRangeCombiner {
       const auto& left = ranges[i];
       const auto& right = ranges[i + 1];
       DCHECK_LE(left.offset, right.offset);
-      DCHECK_LE(left.offset + left.length, right.offset) << "Some read ranges overlap";
+      if (left.offset + left.length > right.offset) {
+        return Status::IOError("Some read ranges overlap");
+      }
     }
 #endif
 
@@ -509,9 +511,9 @@ struct ReadRangeCombiner {
 
 };  // namespace
 
-std::vector<ReadRange> CoalesceReadRanges(std::vector<ReadRange> ranges,
-                                          int64_t hole_size_limit,
-                                          int64_t range_size_limit) {
+Result<std::vector<ReadRange>> CoalesceReadRanges(std::vector<ReadRange> ranges,
+                                                  int64_t hole_size_limit,
+                                                  int64_t range_size_limit) {
   DCHECK_GT(range_size_limit, hole_size_limit);
 
   ReadRangeCombiner combiner{hole_size_limit, range_size_limit};

diff --git a/cpp/src/arrow/io/util_internal.h b/cpp/src/arrow/io/util_internal.h
@@ -45,9 +45,9 @@ ARROW_EXPORT Status ValidateWriteRange(int64_t offset, int64_t size, int64_t fil
 ARROW_EXPORT Status ValidateRange(int64_t offset, int64_t size);
 
 ARROW_EXPORT
-std::vector<ReadRange> CoalesceReadRanges(std::vector<ReadRange> ranges,
-                                          int64_t hole_size_limit,
-                                          int64_t range_size_limit);
+Result<std::vector<ReadRange>> CoalesceReadRanges(std::vector<ReadRange> ranges,
+                                                  int64_t hole_size_limit,
+                                                  int64_t range_size_limit);
 
 ARROW_EXPORT
 ::arrow::internal::ThreadPool* GetIOThreadPool();

diff --git a/cpp/src/parquet/file_reader.cc b/cpp/src/parquet/file_reader.cc
@@ -367,21 +367,6 @@ class SerializedFile : public ParquetFileReader::Contents {
             ComputeColumnChunkRange(file_metadata_.get(), source_size_, row, col));
       }
     }
-#ifndef NDEBUG
-    if (!ranges.empty()) {
-      auto copied_ranges = ranges;
-      std::sort(copied_ranges.begin(), copied_ranges.end(),
-                [](const ::arrow::io::ReadRange& a, const ::arrow::io::ReadRange& b) {
-                  return a.offset < b.offset;
-                });
-      for (size_t i = 1; i < copied_ranges.size(); ++i) {
-        if (copied_ranges[i].offset <
-            copied_ranges[i - 1].offset + copied_ranges[i - 1].length) {
-          throw ParquetException("Overlapping column chunk ranges for pre-buffer");
-        }
-      }
-    }
-#endif
     PARQUET_THROW_NOT_OK(cached_source_->Cache(ranges));
   }