feat: add fallible iterator utility - #905
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A few small but important correctness/maintainability gaps remain (misleading portability comment, unnecessary public-header include, and missing test case for error propagation after partial consumption).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces a reusable fallible, pull-based iceberg::Iterator<T> utility to support lazy production of values with error propagation, and adds focused unit tests to validate ToVector() collection across value-type categories.
Changes:
- Add a new public
iceberg::Iterator<T>interface with aNext()contract returningResult<std::optional<T>>and aToVector()helper that supports move-only and copy-only element types. - Install the new public header via Meson and forward-declare
Iterator<T>intype_fwd.hfor downstream usage. - Add GTest coverage for
ToVector()behavior on copy-only values, move-only values, and error propagation.
File summaries
| File | Description |
|---|---|
| src/iceberg/util/meson.build | Installs the new public iterator header in Meson builds. |
| src/iceberg/util/iterator.h | Adds Iterator<T> interface and ToVector() collection helper for fallible iteration. |
| src/iceberg/type_fwd.h | Adds a forward declaration for Iterator<T>. |
| src/iceberg/test/meson.build | Registers the new iterator unit test in Meson test targets. |
| src/iceberg/test/iterator_test.cc | Adds unit tests for ToVector() across copy-only/move-only types and error propagation. |
| src/iceberg/test/CMakeLists.txt | Registers the new iterator unit test in CMake test targets. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The API is small and self-contained, the error/collection semantics are exercised by targeted unit tests, and build integration is updated for both Meson and CMake.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
How about using |
What changed
Iterator<T>whose reads can failToVector()support for move-only and copy-only value typesWhy
This provides a reusable lazy iteration primitive without coupling it to table scan planning. It is extracted from #873 so the iterator contract and the lazy scan implementation can be reviewed independently.
PR #873 will consume this utility for lazy manifest and file scan task planning.
Testing
ToVector()instantiations for copy-only and move-only values