Skip to content

feat(cpp/python): expose the parallel read/write switches - #1092

Merged
ColinLeeo merged 6 commits into
developfrom
feat/parallel-toggles
Nov 13, 2026
Merged

ColinLeeo merged 6 commits into
developfrom
feat/parallel-toggles

Conversation

@ColinLeeo

Copy link
Copy Markdown
Contributor

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

  • C API: tsfile_set_parallel_read_enabled, tsfile_set_parallel_write_enabled, tsfile_set_thread_count.
  • Python: 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

  • Full C++ suite: 1454 tests passed.
  • Python suite: 607 passed, 16 skipped; the three new functions verified importable and callable.

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-commenter

codecov-commenter commented Nov 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.56%. Comparing base (90c7862) to head (eb8d1bf).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
cpp/src/common/global.cc 80.00% 1 Missing and 1 partial ⚠️
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.
📢 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.

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.
@ColinLeeo
ColinLeeo requested a balanced review from Copilot November 12, 2026 14:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread cpp/src/common/global.cc Resolved
Comment thread python/tsfile/tsfile_py_cpp.pyx Resolved
@ColinLeeo
ColinLeeo requested a lite review from Copilot November 12, 2026 14:56
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread python/tsfile/tsfile_cpp.pxd Resolved
Comment thread cpp/src/common/global.cc Resolved
Comment thread python/tsfile/tsfile_py_cpp.pyx Resolved
Comment thread python/tsfile/tsfile_py_cpp.pyx Resolved
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.
@ColinLeeo
ColinLeeo merged commit fdf3808 into develop Nov 13, 2026
52 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants