Repository navigation
Conversation
|
👋 This is my first change request to the repository, I'm happy to iterate based on any feedback. |
032fc58 to
2f69c1b
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
2f69c1b to
d3aedd8
Compare
|
👋 @mbutrovich @blackmwk tagging you since you were involved in the review of the previous metrics-related PR that this builds off. Could one of you have a look at the changes here? I'd also like to work on getting more planning-stage metrics in, so any feedback on this PR would be very helpful before I start work on that. |
| data_bytes_read: Arc<AtomicU64>, | ||
| delete_bytes_read: Arc<AtomicU64>, | ||
| data_files_opened: Arc<AtomicU64>, | ||
| delete_files_opened: Arc<AtomicU64>, | ||
| rows_emitted: Arc<AtomicU64>, |
There was a problem hiding this comment.
I prefer to do have sth as following:
struct ScanMtricsInnter {
data_bytes_read: AtomicU64,
....
}
pub struct ScanMetrics {
inner: Arc<ScanMtricsInnter>
}
impl ScanMetrics {
pub(crate) fn add_data_byte_read(&self, v: u64) {
...
}
}The benefit of this approach is the the clone only increment Arc's inner counter once.
There was a problem hiding this comment.
👋 Thank you for the review!
I had started with this initially, I left a comment about why I didn't pursue this in my PR description
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.
If the passing of the type of file into the ArrowReader is okay, I can make this change
There was a problem hiding this comment.
Seems the clone is everywhere, could you have a try with that direction? If we go with this direction, we need to remove the Clone of ScanMetrics, which is wasteful, but it may introduce a lot of changes. I want to see what it looks like if we pass ScanMetrics around.
There was a problem hiding this comment.
Yes, I'll change the approach to that
| let file_io = self.file_io.clone(); | ||
| let file_size_in_bytes = task.file_size_in_bytes(); | ||
| let bytes_read = Arc::clone(self.scan_metrics.bytes_read_counter()); | ||
| let bytes_read = Arc::clone(self.scan_metrics.data_bytes_read_counter()); |
There was a problem hiding this comment.
| let bytes_read = Arc::clone(self.scan_metrics.data_bytes_read_counter()); | |
| let data_file_bytes_read = Arc::clone(self.scan_metrics.data_bytes_read_counter()); |
| // batches (using the reader-produced `_pos` position) before the transformer, which | ||
| // then passes it through as a virtual field. | ||
| let first_row_id = task.first_row_id(); | ||
| let scan_metrics = self.scan_metrics.clone(); |
There was a problem hiding this comment.
You only need to clone counter here.
|
|
||
| /// Wraps a [`FileRead`] to count bytes read via a shared atomic counter. | ||
| /// The kind of file whose reads are being counted. | ||
| pub(crate) enum ScanFileType { |
There was a problem hiding this comment.
Why not reuse DataContentType?
There was a problem hiding this comment.
Ah, I didn't see that existed. Changing to use it
… ScanMetrics module
3fc171d to
11e81eb
Compare
|
👋 @blackmwk thank you for the previous reviews; I've updated the PR based on the suggestions |
| } | ||
|
|
||
| /// Wraps a transparently decrypting reader over the given `raw_reader` | ||
| pub(crate) fn decrypt_reader( |
There was a problem hiding this comment.
I perfer the original wrap version, e.g. providing a transform function. This ensures that the file is correctly deciphering the right file.
There was a problem hiding this comment.
Also please add comments to explain why we need this.
There was a problem hiding this comment.
https://github.com/apache/iceberg-rust/pull/3283/changes#diff-a9357ed332174dbf3271413633da29990c6357924a5a8fc50d1e58740ba5dd5dR438-R441 I did add some comments where it's being used, I'll port them here as well.
Could you help me understand this
I prefer the original wrap version, e.g. providing a transform function. This ensures that the file is correctly deciphering the right file.
Is this because there is now a possibility of a different file reader being passed into the function, instead of using the one that the struct was initialized with?
What changes are included in this PR?
This PR adds more detailed metrics to
ArrowReaderscans, building on the initial scan metrics introduced in #2349data_bytes_readdelete_bytes_readdata_files_openeddelete_files_openedrows_emittedbytes_readbytes_readhas a minor change - it is no longer a dedicated metric, and is instead computed fromdata_bytes_read+delete_bytes_read. Sincedelete_bytes_readnow includes bytes read from Puffin files/delete vectors, thebytes_readmetric 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
innerfield inScanMetricsand passing the entirety ofScanMetricsaround, 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 theArrowReaderthat 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
Counterstruct that wraps over theArc<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?
Yes, tests have been added/updated to assert on the newly added metric counters.
AI Disclosure
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.