Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The guard comprehensively covers index reads, and regression tests exercise the relevant closure paths.
0 open findings
What changed in this PR
Adds consistent closed-state handling for memory-mapped dataset indexes.
Changes:
- Guards all index read paths with a clear
RuntimeError. - Adds regression coverage for explicit, contextual, and runtime-driven closure.
| File | Description |
|---|---|
python/tsfile/dataset/index.py |
Adds shared open-state validation to index APIs. |
python/tests/test_dataset_index.py |
Tests closed-index access and runtime teardown scenarios. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reading a
MappedDatasetIndexafter it has been closed can raiseTypeError: a bytes-like object is required, not 'NoneType'when unpacking the cleared mmap view. Accelerated lookups can instead raiseAttributeErrorbecause the Cython lookup has been cleared. These exceptions hide the actual cause and prevent callers from reliably handling closed resources withexcept RuntimeError.The same issue occurs when releasing the final runtime lease closes the index. For example:
This change uses the existing
_view is Nonestate to reject reads withRuntimeError("Dataset Index is closed"). A shared_assert_open()guard covers record, string, count, and accelerated metadata lookup access, and prevents re-entering a closed index. Repeated calls toclose()remain safe.Regression tests cover:
Validation from
python/:python3.12 -m pytest -q tests/test_dataset_index.py tests/test_tsfile_dataset.py— 218 passed.python3.12 -m black --check tsfile/dataset/index.py tests/test_dataset_index.py— passed with Black 26.3.1.git diff --check— passed.