test: simplify IO tests by wjones127 · Pull Request #5228 · lance-format/lance

wjones127 · 2025-11-12T19:09:05Z

This PR makes it easier to make assertions about IO

Make IO statistics on by default.
Make IO statistics tracked even for local object reader (which previously bypassed statistics)
Expose IO stats in Python

codecov-commenter · 2025-11-12T19:47:30Z

Codecov Report

❌ Patch coverage is 86.84211% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.05%. Comparing base (64168b4) to head (3542f68).

Files with missing lines	Patch %	Lines
rust/lance-io/src/object_store.rs	69.69%	10 Missing ⚠️
rust/lance-io/src/utils/tracking_store.rs	85.71%	3 Missing ⚠️
rust/lance-io/src/local.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5228      +/-   ##
==========================================
+ Coverage   82.01%   82.05%   +0.03%     
==========================================
  Files         342      342              
  Lines      141522   141532      +10     
  Branches   141522   141532      +10     
==========================================
+ Hits       116073   116130      +57     
+ Misses      21611    21562      -49     
- Partials     3838     3840       +2

Flag	Coverage Δ
unittests	`82.05% <86.84%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Previously, IO statistics were only available in Rust via the IOTracker wrapper. This adds a Python API to access IO stats through dataset.io_stats(). The implementation includes: - New IoStats pyclass in python/src/dataset/io_stats.rs - io_stats() method on Dataset that returns incremental stats - Python wrapper in LanceDataset class with documentation - Refactored all tests to use dataset.object_store().io_stats() instead of explicit IOTracker instances 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

This commit addresses issues (1), (2), and (3) from the code review: **Issue 1**: Gate unused field behind test-util feature - Added #[cfg(feature = "test-util")] to IoTrackingMultipartUpload::path field - This eliminates the unused field warning in production builds **Issue 2**: Add both snapshot and incremental IO stats methods - Added IOTracker::stats() for non-resetting reads (returns clone) - Renamed ObjectStore::io_stats() to io_stats_incremental() - Added ObjectStore::io_stats_snapshot() for non-resetting reads - Updated all call sites (47 locations) to use io_stats_incremental() - Python API now has: - io_stats_snapshot(): Read-only, doesn't reset counters - io_stats_incremental(): Returns delta and resets counters **Issue 3**: Python type hints - TYPE_CHECKING was already properly configured - IOStats type hint works correctly with existing imports The new API makes the resetting behavior explicit in method names, improving clarity and preventing confusion about when counters reset. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Previously, local filesystem operations bypassed the IO tracking layer because LocalObjectReader reads directly from file handles instead of going through the object_store layer. This commit adds IO tracking for local filesystem reads: **Changes**: - Added `io_tracker: Option<Arc<IOTracker>>` field to `LocalObjectReader` - Added `IOTracker::record_read()` public method for direct recording - Added `LocalObjectReader::open_with_tracker()` internal method - Updated `ObjectStore::open()` and `open_with_size()` to pass IOTracker - Modified `get_range()` and `get_all()` to record operations after reads - Backward compatible: existing direct calls to `LocalObjectReader::open()` still work (tracker is optional) **Testing**: Verified with Python test showing: - Local file reads are now tracked (4 IOPs, 26986 bytes for 1000 rows) - Incremental tracking works correctly - Both snapshot and incremental APIs work for local files This ensures consistent IO tracking across all storage backends (local, S3, GCS, Azure, etc.) giving users complete visibility into their IO operations regardless of where data is stored. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

wjones127 · 2025-11-13T22:44:45Z

@westonpace this is meant to address your comment in #4923 (review)

wkalt · 2025-11-18T17:09:45Z

+    ///
+    /// This metric helps understand IO parallelism. A lower number indicates
+    /// more parallel IO operations.
+    pub num_hops: u64,


calling these hops/network hops isn't clear to me. Would this be network hops behind the S3 endpoint? That's how I would interpret it but I don't think we have that info

is this just network requests?

Maybe I should give an example in the comment.

Imagine a process:

Call list to get 10 files

In parallel, call head on 10 files

Read the largest file

That's a total of 12 requests (list, 10 heads, 1 get). But we do them in 3 "hops". Maybe that's not the best term. Could do "stages" or something else.

I see. Makes me think of dependency chains - not sure if that's a helpful term.

I agree that "hops" is not quite the correct term (though I am used to our usage of it in this way from the Rust unit tests).

I also am not aware of any standard term.

Is this something we want to expose? Should we mention it is likely meaningless if there are concurrent operations in flight? Or that it can be a somewhat noisy metric?

I think I'll rename it for num_stages. You're also right it's mostly useful for testing. So I'll gate it under test-utils.

wkalt · 2025-11-18T17:12:51Z

-        })
+        });
+
+        // Record the read operation if tracking is enabled


is the optionality required? Elsewhere in the PR it seems to indicate it'll always be enabled

Same question.

I can remove it. There is a constructor that doesn't pass a tracker down (used for tests I think). But I can just make it create an empty stats instance and record unconditionally. That can simplify some code.

westonpace

Are these counters process-wide or dataset-wide?

A few questions but no real concerns.

westonpace · 2025-11-18T21:06:10Z

+/// IO statistics for dataset operations
+///
+/// This tracks the number of IO operations and bytes transferred for read and write
+/// operations performed on the dataset's object store.
+///
+/// Note: Calling `io_stats()` returns the statistics accumulated since the last call
+/// and resets the internal counters (incremental stats pattern).
+#[pyclass(name = "IOStats", module = "_lib", get_all)]
+#[derive(Clone, Debug)]
+pub struct IoStats {


It would be nice if we could include these docs in mkdocs. I'll make an issue to figure that out. Then we wouldn't need the lengthy Returns block on the python.

westonpace · 2025-11-18T21:08:08Z

+    ///
+    /// This metric helps understand IO parallelism. A lower number indicates
+    /// more parallel IO operations.
+    pub num_hops: u64,


I agree that "hops" is not quite the correct term (though I am used to our usage of it in this way from the Rust unit tests).

I also am not aware of any standard term.

Is this something we want to expose? Should we mention it is likely meaningless if there are concurrent operations in flight? Or that it can be a somewhat noisy metric?

westonpace · 2025-11-18T21:09:46Z

-        })
+        });
+
+        // Record the read operation if tracking is enabled


Same question.

westonpace · 2025-11-18T21:11:07Z

+        #[cfg(not(feature = "test-util"))]
+        let _ = (method, path); // Suppress unused variable warnings


What are we feature gating here? Tracking of every request's path / method / range vs. just tracking the counts?

Yeah with test-util enabled, we will track a list of all requests made in the IO stats. This makes it much easier to debug a failing test.

But for normal usage, keeping track of those will just accumulate a lot of data for now reason.

This particular gate is just because method and path are only used for the request tracking, so added a line to suppress the unused variable warning.

This PR makes it easier to make assertions about IO * Make IO statistics on by default. * Make IO statistics tracked even for local object reader (which previously bypassed statistics) * Expose IO stats in Python --------- Co-authored-by: Claude <noreply@anthropic.com>

github-actions Bot added the chore label Nov 12, 2025

github-actions Bot added the python label Nov 13, 2025

wjones127 and others added 5 commits November 13, 2025 12:30

simplify IO tests

df5678b

cleanup

5fb6d64

wjones127 force-pushed the feat/simplify-io-tests branch from f26cb88 to 5fb6d64 Compare November 13, 2025 20:38

wjones127 added 3 commits November 13, 2025 13:56

fix s3 test

a7546e0

minor fix

9c988cd

simplify

3542f68

wjones127 marked this pull request as ready for review November 14, 2025 00:10

wjones127 mentioned this pull request Nov 17, 2025

refactor: write bitmap index statistics in file instead #5251

Merged

wkalt reviewed Nov 18, 2025

View reviewed changes

wkalt approved these changes Nov 18, 2025

View reviewed changes

wjones127 requested a review from westonpace November 18, 2025 18:56

westonpace approved these changes Nov 18, 2025

View reviewed changes

wjones127 added 3 commits November 20, 2025 11:17

pr feedback

1a8cc1d

better naming

eac2b74

fix python

94a5088

wjones127 merged commit 1fa26ac into lance-format:main Nov 20, 2025
24 of 25 checks passed

wjones127 deleted the feat/simplify-io-tests branch November 20, 2025 22:38

andrea-reale mentioned this pull request Mar 30, 2026

emilk/fix write starvation rerun-io/lance#12

Closed

		#[cfg(not(feature = "test-util"))]
		let _ = (method, path); // Suppress unused variable warnings

Conversation

wjones127 commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wjones127 commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wjones127 commented Nov 12, 2025 •

edited

Loading

codecov-commenter commented Nov 12, 2025 •

edited

Loading