feat: enhance ManifestReader with projection and filtering support #431

wgtmac · 2025-12-22T14:34:35Z

Refactors the ManifestReader implementation to support advanced data skipping
and projection pushdown, and adds full test coverage ported from the Java implementation.

Implementation Changes:

Consolidate ManifestReader logic: Move implementation from manifest_reader_internal.cc
to manifest_reader.cc and remove the obsolete internal class.
Fluent API: Add Select(), FilterPartitions(), and FilterRows() to support
column projection and expression-based filtering (partition & metrics).
Optimization: Support lazy initialization of the underlying Avro reader.
ManifestEntry: Update schema handling to support reading specific columns and stats.

Test Coverage:

Add manifest_reader_test.cc: Ports TestManifestReader from Java, covering:
- Basic reading and partition filtering.
- V2/V3 specific features: Delete Files and Deletion Vectors (DVs).
- Invalid usage validation (missing snapshot ID).
Add manifest_reader_stats_test.cc: Ports TestManifestReaderStats, verifying:
- Metrics projection logic (stats columns included/dropped based on filter/select).
- Exact match of stats parsing against expected values.
Parameterized Testing: All tests run against Format Versions 1, 2, and 3 to ensure cross-version compatibility.

wgtmac · 2025-12-23T04:36:54Z

src/iceberg/manifest/manifest_reader.cc


+namespace {
+
+#define PARSE_PRIMITIVE_FIELD(item, array_view, type)                                   \


Github is not clever enough to figure out changes in this file. Most parts come from combining manifest_reader.cc and manifest_reader_internal.cc.

Can we change these macros to template functions?

I agree with you. But I prefer to do it in a separate PR.

- Consolidate `ManifestReader` implementation into `manifest_reader.cc` and remove `manifest_reader_internal.cc`. - Implement fluent API for column selection, partition filtering, and row filtering. - Support lazy initialization of the underlying Avro reader. - Add various filtering support for entries.

WZhuo · 2025-12-23T09:32:47Z

src/iceberg/manifest/manifest_reader_internal.h

+
+  /// \brief Add stats columns to the column list if needed.
+  std::vector<std::string> WithStatsColumns(
+      const std::vector<std::string>& columns) const;


It's a static function in ManifestReader

It seems to be a function declaration without implementation. I guess there is a careless mistake.

It is due to a half-way refactoring that I forgot to delete it from the original place.

WZhuo · 2025-12-23T10:18:32Z

src/iceberg/manifest/manifest_reader.h

 class ICEBERG_EXPORT ManifestReader {
 public:
+  /// \brief Special value to select all columns from manifest files.
+  inline static const std::vector<std::string> kAllColumns{"*"};


There's no need to be a vector, and it's also defined in Schema::Select

WZhuo · 2025-12-23T10:20:11Z

src/iceberg/manifest/manifest_reader.cc

+  if (std::ranges::all_of(ManifestReader::kAllColumns,
+                          [&columns](const std::string& col) {
+                            return std::ranges::find(columns, col) != columns.cend();
+                          })) {


Suggested change

if (std::ranges::all_of(ManifestReader::kAllColumns,

[&columns](const std::string& col) {

return std::ranges::find(columns, col) != columns.cend();

})) {

if (std::ranges::find(columns, ManifestReader::kAllColumns) != columns.cend()) {

Use std::ranges::contains instead.

WZhuo · 2025-12-23T10:49:12Z

src/iceberg/manifest/manifest_reader.cc

+  if (std::ranges::all_of(
+          ManifestReader::kAllColumns,
+          [&selected](const std::string& col) { return selected.contains(col); })) {
+    return false;
+  }


Suggested change

if (std::ranges::all_of(

ManifestReader::kAllColumns,

[&selected](const std::string& col) { return selected.contains(col); })) {

return false;

}

if (selected.contains(ManifestReader::kAllColumns)) {

return false;

}

If kAllColumns is a string

HuaHuaY · 2025-12-23T11:10:58Z

src/iceberg/manifest/manifest_entry.h

  /// present
  std::optional<int64_t> content_size_in_bytes;

+  inline static constexpr int32_t kContentFieldId = 134;


We can remove inline on static constexpr.

HuaHuaY · 2025-12-23T11:14:15Z

src/iceberg/manifest/manifest_list.cc


-const StructType& PartitionFieldSummary::Type() {
-  static const StructType kInstance{{
+const std::shared_ptr<StructType>& PartitionFieldSummary::Type() {


Why do we need to change the return type from const T& to const std::shared_ptr<T>&?

Now it is a singleton that can be reused.

HuaHuaY · 2025-12-23T11:15:20Z

src/iceberg/manifest/manifest_reader.cc


+namespace {
+
+#define PARSE_PRIMITIVE_FIELD(item, array_view, type)                                   \


Can we change these macros to template functions?

HuaHuaY · 2025-12-23T11:21:30Z

src/iceberg/manifest/manifest_reader.cc

+  if (std::ranges::all_of(ManifestReader::kAllColumns,
+                          [&columns](const std::string& col) {
+                            return std::ranges::find(columns, col) != columns.cend();
+                          })) {


Use std::ranges::contains instead.

HuaHuaY · 2025-12-23T11:23:47Z

src/iceberg/manifest/manifest_reader.cc

+
+Status ParseLiteral(ArrowArrayView* view_of_partition, int64_t row_idx,
+                    std::vector<ManifestEntry>& manifest_entries) {
+  if (view_of_partition->storage_type == ArrowType::NANOARROW_TYPE_BOOL) {


Use switch instead of multiple if?

HuaHuaY · 2025-12-23T11:30:38Z

src/iceberg/manifest/manifest_reader.cc

+  return manifest_entries;
+}
+
+const std::unordered_set<std::string> kStatsColumns = {


I think std::vector<std::string> is enough.

HuaHuaY · 2025-12-23T11:36:05Z

src/iceberg/manifest/manifest_reader.cc

+    if (result.has_value()) {
+      internal::ArrowArrayGuard array_guard(&result.value());
+      ICEBERG_ASSIGN_OR_RAISE(
+          auto parse_result, ParseManifestList(&arrow_schema, &result.value(), *schema_));
+      manifest_files.insert(manifest_files.end(),
+                            std::make_move_iterator(parse_result.begin()),
+                            std::make_move_iterator(parse_result.end()));
+    } else {
+      // eof
+      break;
+    }


Suggested change

if (result.has_value()) {

internal::ArrowArrayGuard array_guard(&result.value());

ICEBERG_ASSIGN_OR_RAISE(

auto parse_result, ParseManifestList(&arrow_schema, &result.value(), *schema_));

manifest_files.insert(manifest_files.end(),

std::make_move_iterator(parse_result.begin()),

std::make_move_iterator(parse_result.end()));

} else {

// eof

break;

}

if (!result.has_value()) {

// eof

break;

}

internal::ArrowArrayGuard array_guard(&result.value());

ICEBERG_ASSIGN_OR_RAISE(

auto parse_result, ParseManifestList(&arrow_schema, &result.value(), *schema_));

manifest_files.insert(manifest_files.end(),

std::make_move_iterator(parse_result.begin()),

std::make_move_iterator(parse_result.end()));

I prefer to write this way, with one less tab.

WZhuo · 2025-12-23T11:20:53Z

src/iceberg/manifest/manifest_reader.cc

+}
+
+bool ManifestReaderImpl::HasPartitionFilter() const {
+  return part_filter_ && part_filter_->op() != Expression::Operation::kTrue;


part_filter_ always true

Suggested change

return part_filter_ && part_filter_->op() != Expression::Operation::kTrue;

return part_filter_->op() != Expression::Operation::kTrue;

WZhuo · 2025-12-23T11:21:10Z

src/iceberg/manifest/manifest_reader.cc

+}
+
+bool ManifestReaderImpl::HasRowFilter() const {
+  return row_filter_ && row_filter_->op() != Expression::Operation::kTrue;


Suggested change

return row_filter_ && row_filter_->op() != Expression::Operation::kTrue;

return row_filter_->op() != Expression::Operation::kTrue;

zhjwpku · 2025-12-23T10:24:36Z

src/iceberg/manifest/manifest_entry.h

  /// present
  std::optional<int64_t> content_size_in_bytes;

+  inline static constexpr int32_t kContentFieldId = 134;


nit: constexpr implies inline, maybe remove the inline, not a strong opinion

zhjwpku · 2025-12-23T13:07:58Z

src/iceberg/manifest/manifest_reader.cc

+  if (view_of_column->storage_type != ArrowType::NANOARROW_TYPE_LIST) {
+    return InvalidManifestList("partitions field should be a list.");
+  }
+  auto view_of_list_iterm = view_of_column->children[0];


Suggested change

auto view_of_list_iterm = view_of_column->children[0];

auto view_of_list_item = view_of_column->children[0];

typo?

zhjwpku · 2025-12-23T13:09:19Z

src/iceberg/manifest/manifest_reader.cc

+  auto view_of_list_iterm = view_of_column->children[0];
+  // view_of_list_iterm is struct<PartitionFieldSummary>
+  if (view_of_list_iterm->storage_type != ArrowType::NANOARROW_TYPE_STRUCT) {
+    return InvalidManifestList("partitions list field should be a list.");


Suggested change

return InvalidManifestList("partitions list field should be a list.");

return InvalidManifestList("partitions list item should be a struct.");

zhjwpku · 2025-12-23T13:25:11Z

src/iceberg/manifest/manifest_reader.cc

+  return manifest_files;
+}
+
+Status ParseLiteral(ArrowArrayView* view_of_partition, int64_t row_idx,


Should we rename this to ParsePartitionValues, the current name is a little bit confusing to me.

zhjwpku · 2025-12-23T14:09:48Z

src/iceberg/manifest/manifest_reader.h

 class ICEBERG_EXPORT ManifestReader {
 public:
+  /// \brief Special value to select all columns from manifest files.
+  inline static const std::vector<std::string> kAllColumns{"*"};


Why this is a vector, a single string should be enough?

shangxinli · 2025-12-23T14:50:56Z

src/iceberg/manifest/manifest_reader.cc

+      first_row_id_(first_row_id) {}
+
+ManifestReader& ManifestReaderImpl::Select(const std::vector<std::string>& columns) {
+  columns_ = columns;


I think Java maps user-requested names to immutable Field IDs from the manifest's actual schema.

Can we do something like this?

ManifestReader& ManifestReaderImpl::Select(const std::vector<std::string>& columns) { // Validate columns exist in data file schema before storing for (const auto& col_name : columns) { if (col_name != kAllColumns) { ICEBERG_CHECK(DataFile::GetSchemaByName(col_name, case_sensitive_).has_value(), "Column '{}' not found in data file schema", col_name); } } columns_ = columns; return *this; }

shangxinli · 2025-12-23T23:55:55Z

src/iceberg/manifest/manifest_reader.cc

+}
+
+ManifestReader& ManifestReaderImpl::FilterPartitions(std::shared_ptr<Expression> expr) {
+  part_filter_ = expr ? std::move(expr) : True::Instance();


The java implementation have 'AND': https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/ManifestReader.java

shangxinli · 2025-12-23T23:58:04Z

src/iceberg/manifest/manifest_reader.cc

+}
+
+ManifestReader& ManifestReaderImpl::FilterRows(std::shared_ptr<Expression> expr) {
+  row_filter_ = expr ? std::move(expr) : True::Instance();


It is missing AND combination comparing with Java.

shangxinli · 2025-12-24T00:03:42Z

src/iceberg/manifest/manifest_reader.cc

+  if (!row_filter || row_filter->op() == Expression::Operation::kTrue) {
+    return false;
+  }
+  if (columns.empty()) {


Should this function return true when stats columns are needed for filtering but NOT all selected. Currently it returns true when stats are NOT all selected, which is backwards. This will cause unnecessary stats projection when stats columns ARE already selected.

Something like below:

bool RequireStatsProjection(const std::shared_ptr<Expression>& row_filter, const std::vector<std::string>& columns) { if (!row_filter || row_filter->op() == Expression::Operation::kTrue) { return false; // No row filter, no stats needed } if (columns.empty() || std::ranges::contains(columns, ManifestReader::kAllColumns)) { return false; // All columns selected, stats already included } // Return true if ANY stats column is missing from selection const std::unordered_set<std::string> selected(columns.cbegin(), columns.cend()); return !std::ranges::all_of(kStatsColumns, [&selected](const std::string& col) { return selected.contains(col); }); }

shangxinli · 2025-12-24T00:16:03Z

src/iceberg/manifest/manifest_reader.cc

+}
+
+Result<Evaluator*> ManifestReaderImpl::GetEvaluator() {
+  if (!evaluator_) {


Add null checks for row_filter_ and part_filter_ before us?

shangxinli · 2025-12-24T00:17:35Z

src/iceberg/manifest/manifest_reader.h

 class ICEBERG_EXPORT ManifestReader {
 public:
+  /// \brief Special value to select all columns from manifest files.
+  inline static const std::string kAllColumns = "*";


Consider to use constexpr std::string_view for better performance?

wgtmac force-pushed the enhance_manifest_reader branch 3 times, most recently from 537a136 to d4f7c24 Compare December 23, 2025 04:27

wgtmac marked this pull request as ready for review December 23, 2025 04:30

wgtmac force-pushed the enhance_manifest_reader branch from d4f7c24 to 81b1732 Compare December 23, 2025 04:35

wgtmac commented Dec 23, 2025

View reviewed changes

wgtmac force-pushed the enhance_manifest_reader branch from 81b1732 to 5b11c46 Compare December 23, 2025 04:39

wgtmac changed the title ~~feat: add projection and filtering to manifest reader~~ feat: enhance ManifestReader with projection, filtering and comprehensive tests Dec 23, 2025

wgtmac changed the title ~~feat: enhance ManifestReader with projection, filtering and comprehensive tests~~ feat: enhance ManifestReader with projection and filtering support Dec 23, 2025

WZhuo reviewed Dec 23, 2025

View reviewed changes

HuaHuaY reviewed Dec 23, 2025

View reviewed changes

WZhuo reviewed Dec 23, 2025

View reviewed changes

address feedback

33a736b

zhjwpku reviewed Dec 23, 2025

View reviewed changes

shangxinli reviewed Dec 23, 2025

View reviewed changes

shangxinli reviewed Dec 24, 2025

View reviewed changes


		namespace {

		#define PARSE_PRIMITIVE_FIELD(item, array_view, type) \

	return part_filter_ && part_filter_->op() != Expression::Operation::kTrue;
	return part_filter_->op() != Expression::Operation::kTrue;

	return row_filter_ && row_filter_->op() != Expression::Operation::kTrue;
	return row_filter_->op() != Expression::Operation::kTrue;

	auto view_of_list_iterm = view_of_column->children[0];
	auto view_of_list_item = view_of_column->children[0];

	return InvalidManifestList("partitions list field should be a list.");
	return InvalidManifestList("partitions list item should be a struct.");

feat: enhance ManifestReader with projection and filtering support #431

Are you sure you want to change the base?

feat: enhance ManifestReader with projection and filtering support #431

Conversation

wgtmac commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation Changes:

Test Coverage:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuaHuaY Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WZhuo Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WZhuo Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

wgtmac commented Dec 22, 2025 •

edited

Loading

HuaHuaY Dec 23, 2025 •

edited

Loading

WZhuo Dec 23, 2025 •

edited

Loading

WZhuo Dec 23, 2025 •

edited

Loading