[Feature][C++][Python] Support seekable random-access TsFile sources - #939
Conversation
baf4a55 to
d846555
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated Python TsFileReader constructor currently treats non-str path-like inputs (e.g., pathlib.Path) as file-like objects and raises a misleading error instead of cleanly accepting os.PathLike paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a seekable random-access I/O abstraction to the C++ TsFile reader stack and exposes it through the Python bindings so TsFileReader can consume remote/seekable binary file objects (e.g., via fsspec) without changing the existing path-based C API.
Changes:
- Introduces a C++
RandomAccessFileinterface (withReadFileas the local implementation) and updates the reader/query stack to read through it. - Extends Python
TsFileReaderto accept either a filesystem path or a seekable binary file-like object via a small C++ bridge. - Adds Python tests plus README documentation (EN + ZH) for file-like/remote reading usage.
File summaries
| File | Description |
|---|---|
| python/tsfile/tsfile_reader.pyx | Accepts either path strings or seekable file-like objects and wires to the new bridge constructor. |
| python/tsfile/python_random_access_file.h | Declares the Python-to-C++ reader factory used by Cython. |
| python/tsfile/python_random_access_file.cc | Implements a RandomAccessFile backed by Python seek/tell/read while preserving cursor and ownership. |
| python/tests/test_file_like_reader.py | Adds coverage for cursor preservation, lifetime, short reads, and error propagation for file-like sources. |
| python/setup.py | Builds tsfile_reader with the new C++ bridge source and header dependency. |
| python/README.md | Documents using TsFileReader with seekable binary file objects (e.g., fsspec). |
| python/README-zh.md | Chinese documentation for seekable binary file object support. |
| python/pyproject.toml | Ships the new bridge header as package data. |
| python/pom.xml | Excludes the shipped header from the pom-managed file set as needed. |
| cpp/test/reader/tsfile_reader_test.cc | Adds a RandomAccessFile-backed test and validates move semantics/meta-arena access. |
| cpp/test/reader/table_view/table_model_encoding_compression_compatibility_test.cc | Updates includes for refactored file access types. |
| cpp/test/reader/chunk_reader_resource_test.cc | Updates includes for refactored file access types. |
| cpp/src/reader/tsfile_series_scan_iterator.h | Switches iterator dependencies from ReadFile to RandomAccessFile. |
| cpp/src/reader/tsfile_series_scan_iterator.cc | Updates implementations to match RandomAccessFile usage. |
| cpp/src/reader/tsfile_reader.h | Adds open(std::unique_ptr<RandomAccessFile>), move operations, and internal ownership changes. |
| cpp/src/reader/tsfile_reader.cc | Implements open/close through RandomAccessFile, validation, and error-return improvements. |
| cpp/src/reader/tsfile_executor.h | Updates init signature to accept RandomAccessFile and adds a checked metadata accessor. |
| cpp/src/reader/tsfile_executor.cc | Implements the updated RandomAccessFile-based init. |
| cpp/src/reader/table_query_executor.h | Updates executor to accept RandomAccessFile. |
| cpp/src/reader/query_executor.h | Updates includes/comments for the new file abstraction. |
| cpp/src/reader/ichunk_reader.h | Updates reader interface to accept RandomAccessFile. |
| cpp/src/reader/chunk_reader.h | Switches chunk reader to use RandomAccessFile. |
| cpp/src/reader/chunk_reader.cc | Updates implementation signatures accordingly. |
| cpp/src/reader/aligned_chunk_reader.h | Switches aligned chunk reader to use RandomAccessFile. |
| cpp/src/reader/aligned_chunk_reader.cc | Updates signatures and comments to match RandomAccessFile semantics. |
| cpp/src/file/tsfile_io_reader.h | Migrates internal file pointer type and adds a checked metadata accessor. |
| cpp/src/file/tsfile_io_reader.cc | Updates initialization paths and helper functions to use RandomAccessFile. |
| cpp/src/file/read_file.h | Makes ReadFile implement RandomAccessFile. |
| cpp/src/file/read_file.cc | Delegates magic/version validation to shared validate_tsfile(). |
| cpp/src/file/random_access_file.h | Adds the new interface and validate_tsfile() declaration. |
| cpp/src/file/random_access_file.cc | Implements shared TsFile validation for any RandomAccessFile. |
| .gitignore | Ensures the new shipped header is not ignored. |
Review details
- Files reviewed: 31/32 changed files
- Comments generated: 1
- 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.
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 47 out of 48 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
python/tsfile/tsfile_reader.pyx:1
TsFileReaderpreviously accepted a “pathname”, but the newisinstance(source, str)gate will reject common path-like inputs (e.g.,pathlib.Path,numpy.str_, or objects implementingos.PathLike) and incorrectly route them through the file-object path (raisingTypeError). Consider normalizing viaos.fspath(source)whenisinstance(source, (str, bytes, os.PathLike)), to preserve compatibility while still supporting seekable binary objects.
cpp/src/reader/tsfile_reader.cc:1get_all_device_ids()now calls the error-reporting overload (get_all_devices(device_ids)) but discards its return code, so a device-index read failure can silently turn into an empty device list and produce a “successful” empty result. To ensure read failures propagate (as per the PR’s error-propagation goal), use the error-reporting overload directly here, check its return value, and abort the query on failure.
/*
python/tsfile/python_random_access_file.cc:1
seek_to(...)and its callers use raw integers forwhence(e.g.,0and2). Replacing these withSEEK_SET/SEEK_ENDimproves readability and reduces the chance of passing an incorrect constant when extending the code.
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-language I/O, concurrency, and error-propagation refactors are broad enough that they warrant final human review despite strong test coverage.
Review details
Suppressed comments (3)
python/tsfile/python_random_access_file.cc:267
- On
seek()failure during size probing, the code restores the cursor but keeps the Python exception set and returnserror_code. With the Cythonexcept? NULLsignature, the raw Python exception will be raised instead of mapping to the existing open/read error codes.
python/tsfile/python_random_access_file.cc:274 - If
tell()fails while computing the file size, this returnserror_codebut leaves the Python exception set, so Cython will propagate the raw exception and skipcheck_error()mapping. Clearing the Python error here keeps the public error surface consistent with other failures mapped viaerror_code.
python/tsfile/python_random_access_file.cc:277 - If restoring the original cursor position fails, this returns
error_codebut leaves the Python exception set, which will bypass the intendederror_codehandling in the Cython caller (due toexcept? NULL). Clearing the Python error keeps error reporting consistent.
- Files reviewed: 47/48 changed files
- Comments generated: 1
- Review effort level: Lite
| if (!tell_position(source, original_position)) { | ||
| *error_code = common::E_FILE_OPEN_ERR; | ||
| return nullptr; | ||
| } |
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 47 out of 48 changed files in this pull request and generated no new comments.
Suppressed comments (2)
python/tsfile/python_random_access_file.cc:1
- PythonSourceLock mixes the
PyGILState_*API withPyEval_SaveThread/PyEval_RestoreThreadin the same scope. CPython explicitly discourages mixing these two GIL management models because it can lead to incorrect thread-state handling (hard-to-debug crashes/hangs), especially when the calling thread may or may not already own the GIL.\n\nA safer approach is to avoid combining the APIs and instead: (1) lock the mutex without the GIL, then (2) acquire the GIL only for the Python C-API calls (or conditionally release/reacquire the GIL around the mutex lock using a single consistent mechanism). This also lets you avoid acquiring the GIL in methods likeis_opened()/generation()that don’t call into Python at all.
python/tsfile/python_random_access_file.cc:1 - When
read(remaining)returns an empty bytes-like object (view.len == 0) beforerequestedbytes have been satisfied, the code breaks and returnsE_OKwith a shortread_size. Givenrequestedis bounded by the probedsize_andoffset < size_, hitting EOF early likely indicates an inconsistent size probe or a source/read failure and should be surfaced asE_FILE_READ_ERR.\n\nSuggested fix: ifview.len == 0andread_size < requested, setret = common::E_FILE_READ_ERRbefore breaking (or after the loop ifret == E_OKbutread_size != requested). This enforces the RandomAccessFile contract and prevents silent truncation from being misinterpreted as successful reads.
Summary
RandomAccessFileabstraction withLocalRandomAccessFileas the local backend (file/local_random_access_file.h)TsFileReaderto consume seekable binary file objects while keeping the C reader-opening API path-onlyTsFileReadernon-copyable and non-movable, with its metadata arena stored as a value memberfsspecusageTesting
./mvnw clean verify -P with-python(922 C++ tests passed, 3 skipped; 246 Python tests passed)HfFileSystem.open()comparison: 19,701 rows and 29 columns matched a local copy exactlyFixes #930