apache / apache/iceberg-cpp

Add a minimal micro-benchmark suite to validate filtering hot-path optimizations

オープン
#690 コメント 3 件 リアクション 1 件 担当者 0 名 GitHub で見る
主要言語
C++
スター
221
フォーク
124
平均マージ
1日 16時間
マージ済み PR(30日)
21

説明

While reading the scan-planning filtering path, I found a small optimization in the metrics evaluators. Using it as a concrete example to raise a broader question about how to validate this kind of change.

## Proposed change

The metrics evaluators run per data file. Each predicate currently calls `expr->reference()` repeatedly, and `reference()` returns a `shared_ptr` via `shared_from_this()` — an atomic refcount bump every time. The `StrictMetricsEvaluator` macro even discards a `dynamic_cast` result only to re-fetch the same reference:

```diff
- #define RETURN_IF_NOT_REFERENCE(expr) \
- if (auto ref = dynamic_cast(expr.get()); ref == nullptr) { \
- return kRowsMightNotMatch; \
- }
+ #define BIND_REFERENCE_OR_RETURN(ref, expr) \
+ const auto* ref = dynamic_cast((expr).get()); \
+ if (ref == nullptr) { \
+ return kRowsMightNotMatch; \
+ }

Result IsNull(const std::shared_ptr& expr) override {
- RETURN_IF_NOT_REFERENCE(expr);
- int32_t id = expr->reference()->field().field_id();
+ BIND_REFERENCE_OR_RETURN(ref, expr);
+ int32_t id = ref->field().field_id();
...
```

Reusing the cast result drops the repeated virtual `reference()` calls (and their atomic ops) across every predicate, with no behavior change.

## Expected benefit

The win is on the CPU-bound filtering step, evaluated in isolation. Scan planning as a whole is IO-bound, so on an e2e scan this kind of change is almost certainly unmeasurable — which is exactly why it needs to be measured on the filtering step alone.

## Which raises the question: do we need a benchmark suite?

This is exactly the kind of change that's hard to justify without one. The repo has no benchmark infrastructure today, only the gtest suite. A minimal benchmark on the filtering path would let us measure such changes on the CPU-bound step alone, rather than guessing or claiming a win against IO-dominated planning.

So before going further:

1. **How** should we add it — Google Benchmark fetched the same way googletest already is, behind an off-by-default CMake option?
2. **Where** should it live — a top-level `benchmark/`, or co-located under `src/iceberg/**/`?

I'm happy to put up a draft PR for a minimal suite + the filtering benchmark above once there's agreement on direction.

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

まず既存の CMake と GoogleTest のセットアップを確認し、次に src/iceberg/**/ 配下の filtering パスとその metrics evaluators を追跡します。プロジェクトに合意済みの最小限の benchmark suite、デフォルトで無効になっている build option、そして filtering ステップを単独で測定する benchmark が用意されれば、作業は完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
cpp
領域
build-system, performance, tooling
issue の種類
機能追加
難易度
4/5
見積もり時間
3〜5日
活発さ
静か
明瞭さ
説明が足りない
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。