Repository navigation
feat(cpp/python): expose the parallel read/write switches - #1092
Conversation
Add tsfile_set_parallel_read_enabled, tsfile_set_parallel_write_enabled and tsfile_set_thread_count to the C API, and the matching set_parallel_read_enabled / set_parallel_write_enabled / set_thread_count to the Python binding. The switches already existed as C++ globals; only the bindings were missing. They matter for benchmarking and for single-table workloads, where running the read path with the pool disabled measured ~20% faster than the default because the worker pool is mostly idle.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1092 +/- ##
===========================================
+ Coverage 66.51% 66.56% +0.05%
===========================================
Files 778 782 +4
Lines 56577 56733 +156
Branches 9105 9135 +30
===========================================
+ Hits 37633 37767 +134
- Misses 17291 17305 +14
- Partials 1653 1661 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
C API: verify the setters take effect on the shared config, that thread counts outside [1, 64] are rejected without touching the configured value, and that flipping the parallel read switch over the same file returns identical rows. Python: exercise the new bindings, including the InvalidArgumentError raised for an out-of-range thread count, and compare reads with the parallel path on and off.
Follow-ups from reviewing the new switches: * Setting a parallel flag before the first reader/writer was silently undone: init_config_value() assigned both flags true during the lazy initialization performed by tsfile_reader_new/write_file_new. Give them in-class defaults (as thread_count_ already has) and stop assigning them at init time. Regression test sets both to 0, creates a file handle, and checks they stay 0. * set_thread_count() deleted the live pool before allocating its replacement, so a failed thread creation left g_thread_pool_ dangling and let the exception escape the extern "C" boundary. Build the new pool first, swap it in, then delete the old one; map failures to RET_OOM and keep the previous pool. Skip the join/respawn when the count is unchanged. * Add tsfile_get_parallel_read_enabled/..._write_enabled/ tsfile_get_thread_count and the matching Python getters, and surface all three through get_tsfile_config()/set_tsfile_config(). The setter docs now state that the flags are read per operation and that no reader, writer or result set may be active when resizing the pool. * Update cpp/README.md and python/README.md, which documented non-existent entry points (storage::set_write_thread_count) and described auto-disable on single-core machines.
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.
Copilot review overview
2 open issues
What changed in this PR
Expose existing C++ global toggles for parallel read/write and worker thread-pool sizing through the C API and Python bindings so users can benchmark/tune these behaviors without changing defaults.
Changes:
- Add C API getters/setters for parallel read/write toggles and thread count; wire them into the C wrapper implementation.
- Expose the same controls in the Cython/Python layer (including config dict round-tripping).
- Add C++ and Python tests covering validation, round-trip behavior, and “set-before-first-use” lazy-init ordering.
| File | Description |
|---|---|
| python/tsfile/tsfile_py_cpp.pyx | Adds Python-exposed getters/setters and config dict integration for the new switches. |
| python/tsfile/tsfile_py_cpp.pxd | Exports the new Python API functions. |
| python/tsfile/tsfile_cpp.pxd | Declares new C wrapper externs and config fields for Cython access. |
| python/tsfile/__init__.py | Re-exports new APIs at package top-level. |
| python/tests/test_write_and_read.py | Adds coverage for validation, config dict integration, and lazy-init ordering via subprocess. |
| python/README.md | Documents the new Python controls and concurrency constraints. |
| cpp/test/cwrapper/cwrapper_test.cc | Adds C wrapper tests for validation, lazy-init ordering, and read-result equivalence. |
| cpp/src/cwrapper/tsfile_cwrapper.h | Adds C API declarations + documentation for toggles and thread count. |
| cpp/src/cwrapper/tsfile_cwrapper.cc | Implements new C API functions by delegating to common. |
| cpp/src/common/global.cc | Improves set_thread_count to avoid exception propagation and preserve old pool on failure. |
| cpp/src/common/config/config.h | Moves parallel toggle defaults to in-class initialization to survive lazy init. |
| cpp/README.md | Updates documentation for runtime toggles and the global worker pool API. |
Review effort: Lite
Catch (...) when building the replacement pool so no exception can cross the C API boundary, and accept integer-like values (numpy integers, IntEnum) in the Python config setter while still rejecting bool.
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.
Copilot review overview
6 open issues
ConfigValueincpp/src/common/config/config.hdeclaresparallel_read_enabled_/… · New The comment immediately above says to 'never propagate an exception across the C API boundary', but… · New Usingint32_tas the Python-visible parameter type can raiseOverflowErrorduring… · New Usingint32_tas the Python-visible parameter type can raiseOverflowErrorduring… · New_check_int()currently only accepts built-inint, which will reject valid integer-like types… The comment notes exceptions must not cross the C API boundary, but this only catches…
Review effort: Lite
set_thread_count() took int32_t, so Python-int-to-int32 conversion raised OverflowError before the library could return RET_INVALID_ARG. Accept an object, validate it (integer-like, excluding bool) and the [1, 64] range, then convert. Also declare ConfigValue's C++ bool fields as libcpp bool in the pxd instead of bint, so Cython sees the real field type.
Parallel CTest processes can seed the name generator in the same clock tick; one then removes the file another is opening (E_ALREADY_EXIST at the fixture's open, observed as SEGFAULT / test failure on CI). Append pid and a per-process counter, as the writer tests already do.


Problem
The C++ implementation already has global switches for the parallel read/write paths and the worker-pool size, but they were not reachable from the C API or the Python binding, so users could not measure or tune them.
Change
tsfile_set_parallel_read_enabled,tsfile_set_parallel_write_enabled,tsfile_set_thread_count.set_parallel_read_enabled,set_parallel_write_enabled,set_thread_count.No behavior change for existing callers; the defaults are unchanged.
Why it matters
For single-table reads the worker pool is mostly idle: with the parallel read path disabled, a 3.2M-row full scan measured ~20% faster than the default (0.25 s vs 0.21 s). Exposing the switch lets users and benchmarks make that choice, and lets the default be revisited with data.
Testing