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 |
Cache terminal iterator results so repeated calls do not advance implementations, and explicitly enable move operations. Co-authored-by: Codex <codex@openai.com>
@HuaHuaY Can you provide more context? |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
|
@HuaHuaY AFAIK, |
There was a problem hiding this comment.
🟢 Approval recommended
The core API and terminal-state semantics are correct and validated by focused unit tests, with only a non-blocking performance optimization suggested for the copy-only collection path.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
It was my fault for not noticing this. https://cppstat.dev/zh-Hans/?search=std%3A%3Agenerator |
There was a problem hiding this comment.
🟢 Approval recommended
The iterator contract and sticky terminal semantics are implemented coherently and are covered by targeted unit tests for copy-only, move-only, exhaustion, and error propagation.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A newly added public-header comment still states a misleading portability rationale for the copy-only ToVector() path and should be corrected for accuracy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| // Growing a vector requires move-insertable elements in some standard library | ||
| // implementations. Stage strictly copy-only values in a deque, then copy them | ||
| // into an exactly sized vector. |
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