Perseus opened a new pull request, #3283:
URL: https://github.com/apache/iceberg-rust/pull/3283
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes
such as new features, bug fixes, refactoring, or any other updates.
-->
This PR adds more detailed metrics to `ArrowReader` scans, building on the
initial scan metrics introduced in #2349
- **New Metrics**
- `data_bytes_read`
- `delete_bytes_read`
- `data_files_opened`
- `delete_files_opened`
- `rows_emitted`
- **Modified Metrics**
- `bytes_read`
`bytes_read` has a minor change - it is no longer a dedicated metric, and is
instead computed from `data_bytes_read` + `delete_bytes_read`. Since
`delete_bytes_read` now includes bytes read from Puffin files/delete vectors,
the `bytes_read` metric may now report a higher value to existing consumers.
These metrics would be really useful for query engines like DataFusion to
diagnose issues such as small file overhead, delete file amplification/bloat
etc. and just have a generally better understanding of the storage system.
### Design Notes
I'd initially started with moving all metrics to an `inner` field in
`ScanMetrics` and passing the entirety of `ScanMetrics` around, but when it
came to distinguishing between delete-file metrics and data-file metrics, it
required passing context about the type of file (data/delete/...) down into the
`ArrowReader` that I felt like would cause unnecessary coupling.
This led to the current design of separate properties and functions exposing
those. I'm happy to iterate on this based on any feedback/preferences from the
maintainers.
An alternative that I considered was to introduce a `Counter` struct that
wraps over the `Arc<AtomicU64>` and hides the fetch_add/ordering implementation
details from the code that increments this metric, but didn't want to introduce
too many changes in this PR.
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are
they covered by existing tests)?
-->
Yes, tests have been added/updated to assert on the newly added metric
counters.
## AI Disclosure
<!--
https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions
-->
I used AI to understand the codebase and review my changes to try and make
them as idiomatic/guideline-following as possible. I made all the code changes
myself, and had AI generate the tests. I have reviewed the tests myself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
