fix(cpp): propagate table read failures without silent data loss by ColinLeeo · Pull Request #970 · apache/tsfile

ColinLeeo · 2026-09-24T09:27:58Z

Propagate metadata and device-index read errors through table queries and tag filters instead of returning empty or partial results or misleading errors. Preserve terminal result-set failures and reject unexpected short reads while allowing EOF-limited prefetches.

Use checked schema lookups in the C/Python bindings and release tag filters when query creation fails. Add fault-injection regression coverage for row and batch queries, tag filtering, and leaf/internal device indexes.

Validation: C++ 953 passed, 3 skipped; Python 356 passed, 1 skipped. Spotless, Black, and whitespace checks passed.

Propagate metadata and device-index read errors through table queries and tag filters instead of returning empty or partial results or misleading errors. Preserve terminal result-set failures and reject unexpected short reads while allowing EOF-limited prefetches. Use checked schema lookups in the C/Python bindings and release tag filters when query creation fails. Add fault-injection regression coverage for row and batch queries, tag filtering, and leaf/internal device indexes. Validation: C++ 953 passed, 3 skipped; Python 356 passed, 1 skipped. Spotless, Black, and whitespace checks passed.

ColinLeeo · 2026-09-24T10:47:41Z

@@ -177,22 +192,25 @@ std::shared_ptr<ResultSetMetadata> TableResultSet::get_metadata() {
 int TableResultSet::get_next_tsblock(common::TsBlock*& block) {
    int ret = common::E_OK;
    block = nullptr;
+    if (read_error_ != common::E_OK) {
+        return read_error_;
+    }



There are quite a few state transitions here, so I'd prefer to refactor this logic. For example, I'd introduce an explicit state machine and model the read-state transitions separately for row-wise and batch reads.

Remove the legacy no-error table schema and tag factory entry points. Make table and timeseries schema lookups return allocated objects with ERRNO output parameters, including read failures instead of silently returning empty or partial schemas. Update the C++ reader, CLI, examples, Python bindings, Go cgo bridge, and tests to the unified API. Add fault-injection coverage for all-timeseries schema reads. Validation: C++ 953 passed, 3 skipped; Python 357 passed, 1 skipped; Go tests passed. Formatting and whitespace checks passed.

Copilot

Copilot review overview

🟡 Changes recommended

Critical C ABI compatibility and exception-safety issues, plus unresolved CLI and native-memory leaks, block approval.

Review effort: Lite
Findings: 3 · 2 · 1

Open (6)

Restore exception handling and allocation checks · New
Preserve C API and ABI compatibility · New
Update header declaration for filter builder · New
Use error-reporting overload for schema lookup · New
Propagate metadata errors before table lookup · New
Fix typo in license header · New

What changed in this PR

This PR propagates table metadata, device-index, and result-set read failures across C++, Python, Go, and CLI interfaces.

Changes:

Adds checked schema and metadata APIs.
Preserves query failures and rejects unexpected short reads.
Adds fault-injection and binding regression coverage.

File	Summary
`python/tsfile/tsfile_reader.pyx`	Manages tag-filter lifetime.
`python/tsfile/tsfile_py_cpp.pyx`	Uses checked schema APIs.
`python/tsfile/tsfile_cpp.pxd`	Updates C API declarations.
`python/tests/test_reader_sources.py`	Adds read-failure coverage.
`go/tsfile/cgo_bridge.go`	Propagates schema errors.
`cpp/tools/commands/row_query.cc`	Handles schema and filter errors.
`cpp/tools/commands/cmd_stats.cc`	Handles schema lookup errors.
`cpp/tools/commands/cmd_count.cc`	Handles schema lookup errors.
`cpp/test/reader/table_view/tsfile_reader_table_test.cc`	Updates schema tests.
`cpp/test/reader/table_read_failure_test.cc`	Adds failure regression tests.
`cpp/test/cwrapper/cwrapper_test.cc`	Updates wrapper tests.
`cpp/test/cwrapper/c_release_test.cc`	Updates C API cleanup tests.
`cpp/src/reader/tsfile_reader.h`	Defines checked schema lookup.
`cpp/src/reader/tsfile_reader.cc`	Implements checked lookup.
`cpp/src/reader/task/device_task_iterator.h`	Propagates iterator errors.
`cpp/src/reader/task/device_task_iterator.cc`	Propagates iterator errors.
`cpp/src/reader/table_result_set.h`	Tracks terminal read failures.
`cpp/src/reader/table_result_set.cc`	Preserves result-set failures.
`cpp/src/reader/table_query_executor.cc`	Propagates metadata failures.
`cpp/src/reader/meta_data_querier.h`	Removes cached metadata state.
`cpp/src/reader/meta_data_querier.cc`	Uses checked metadata access.
`cpp/src/reader/device_meta_iterator.h`	Tracks index ownership and errors.
`cpp/src/reader/device_meta_iterator.cc`	Propagates index failures.
`cpp/src/reader/chunk_reader.cc`	Rejects unexpected short reads.
`cpp/src/reader/block/device_ordered_tsblock_reader.cc`	Propagates iterator errors.
`cpp/src/reader/aligned_chunk_reader.cc`	Validates aligned reads.
`cpp/src/cwrapper/tsfile_cwrapper.h`	Revises checked C APIs.
`cpp/src/cwrapper/tsfile_cwrapper.cc`	Implements checked wrappers.
`cpp/examples/cpp_examples/demo_read.cpp`	Updates schema API usage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ColinLeeo · 2026-09-29T02:07:45Z

+                                                 uint32_t* size,
+                                                 ERRNO* error_code) {
    if (size != nullptr) {
        *size = 0;
    }


Fixed in commit 764d8ba8a. The C wrapper now checks every calloc/strdup result, validates schema vectors, catches std::bad_alloc and other exceptions, and frees partially initialized schema arrays before returning E_OOM or E_FILE_READ_ERR. The read-failure regression tests cover the metadata APIs.

ColinLeeo · 2026-09-29T02:08:05Z

+TableSchema* tsfile_reader_get_table_schema(TsFileReader reader,
+                                            const char* table_name,
+                                            ERRNO* error_code);


Thanks for pointing this out. For this PR we intentionally accept the breaking API change: backward compatibility is out of scope, and the old signatures could not report metadata read failures reliably. The original exported names now use the error-reporting signatures directly, so the C, Go, and Python bindings share one consistent API without _with_error compatibility shims.

Follow-up: the latest implementation preserves the original exported C APIs and adds checked variants for error propagation. The legacy wrappers delegate to the checked implementations and handle returned errors, so existing source and ABI compatibility are retained. My previous reply describing an intentional breaking change referred to an earlier revision and is no longer accurate.

ColinLeeo · 2026-09-29T02:08:25Z

+int build_table_tag_filter(const ParsedArgs& args,
+                           storage::TsFileReader& reader,
+                           const std::string& table_name, std::ostream& err,
+                           std::unique_ptr<storage::Filter>& ret_filter) {


Fixed. cpp/tools/commands/commands.h now declares the status-code/out-parameter signature used by row_query.cc, and the CLI builds successfully.

ColinLeeo · 2026-09-29T02:08:31Z

+        if (schemas.empty() || !schemas[0]) {
+            err << "Error: table '" << args.table << "' does not exist\n";
+            return kExitUsage;
+        }


Fixed. The no-table path now calls the status-returning reader.get_all_table_schemas(schemas) overload, reports metadata read failures, and returns kExitFile instead of treating a read failure as “no table found”.

ColinLeeo · 2026-09-29T02:08:32Z

+        std::shared_ptr<storage::TableSchema> table_schema;
+        const int schema_ret =
+            reader.get_table_schema(table_name, table_schema);
+        if (schema_ret != common::E_OK) {


Fixed. cmd_table_stats now uses the status-returning schema enumeration overload and propagates metadata read errors before applying the missing-table usage diagnostic.

Correction to my previous reply: row_query now uses the status-returning schema enumeration overload in the no-table path and propagates metadata read errors before reporting that no table was found.

jt2594838 · 2026-09-28T06:23:13Z

-    std::queue<MetaIndexNode*> meta_index_nodes_;
+    // Roots are borrowed from file metadata; descendant nodes belong to pa_.
+    std::queue<std::pair<MetaIndexNode*, bool>> meta_index_nodes_;


What does the bool represent? Add to the comment.

The flag tracks ownership: roots borrowed from file metadata are false, while descendants allocated in pa_ are true and must be explicitly destroyed by the iterator. I added this comment next to the member declaration.

jt2594838 · 2026-09-29T09:19:09Z

+        if (table_num > std::numeric_limits<uint32_t>::max()) {
+            *error_code = common::E_OVERFLOW;
+            return nullptr;
+        }


Why use uint32?

The public C API exposes schema counts as uint32_t, so this implementation keeps the existing ABI and checks the internal size_t count before narrowing. If the count exceeds UINT32_MAX, it returns E_OVERFLOW instead of truncating it.

…-codes # Conflicts: # cpp/tools/commands/cmd_stats.cc

…-codes

ColinLeeo commented Sep 24, 2026

View reviewed changes

ColinLeeo requested review from hongzhi-gao and jt2594838 and a lite review from Copilot and removed request for Copilot September 28, 2026 03:23

Copilot started reviewing on behalf of ColinLeeo September 28, 2026 03:24 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

hongzhi-gao approved these changes Sep 28, 2026

View reviewed changes

fix(cpp): propagate schema read failures safely

764d8ba

jt2594838 reviewed Sep 29, 2026

View reviewed changes

ColinLeeo force-pushed the colin/fix-table-read-error-propagation branch from c586f10 to 764d8ba Compare September 30, 2026 02:54

ColinLeeo added 6 commits September 30, 2026 12:25

fix(cpp): preserve API compatibility with checked errors

db6406c

Merge remote-tracking branch 'origin/develop' into colin/pr-970-error…

cab990d

…-codes # Conflicts: # cpp/tools/commands/cmd_stats.cc

Merge remote-tracking branch 'origin/develop' into colin/pr-970-error…

3cc3acd

…-codes

refactor(cpp): align schema wrapper error handling

bf3bce1

fix(cpp): preserve table time statistics in filtered metadata

dbccee3

refactor(cpp): keep exception handling changes scoped

c83faf9

Navigation Menu

fix(cpp): propagate table read failures without silent data loss - #970

fix(cpp): propagate table read failures without silent data loss#970
ColinLeeo wants to merge 9 commits into
apache:developfrom
ColinLeeo:colin/fix-table-read-error-propagation

ColinLeeo commented Sep 24, 2026

Uh oh!

ColinLeeo Sep 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

ColinLeeo Sep 30, 2026

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

ColinLeeo Sep 29, 2026

Uh oh!

Uh oh!

jt2594838 Sep 28, 2026

Uh oh!

ColinLeeo Sep 30, 2026

Uh oh!

jt2594838 Sep 29, 2026

Uh oh!

ColinLeeo Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ColinLeeo commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants