Repository navigation
fix(cpp): replace malformed UTF-8 in CLI text output - #997
Merged
ColinLeeo merged 2 commits intoOct 8, 2026
Merged
Conversation
Contributor
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
4 open findings
textis computed unconditionally (includingcontent.str()allocation + UTF-8 scan), but when… · Newjson_escape()andtable_escape()currently allocate an intermediate sanitized string via… · Newjson_escape()andtable_escape()currently allocate an intermediate sanitized string via… · New
What changed in this PR
Updates the C++ CLI text output pipeline to sanitize malformed UTF-8 identifiers/strings by replacing ill-formed UTF-8 subparts with U+FFFD at output boundaries, ensuring CSV/NDJSON/table output (plus sketch and export manifests) remains valid UTF-8.
Changes:
- Introduces a shared
replace_invalid_utf8()helper and applies it in CSV/JSON/table escaping. - Sanitizes sketch output and reuses shared JSON escaping for export manifests.
- Adds regression and e2e tests covering invalid UTF-8 sequences while preserving valid UTF-8 and BLOB hex output.
| File | Description |
|---|---|
| cpp/tools/format/output_format.h | Declares shared UTF-8 sanitization helper for text output formatting. |
| cpp/tools/format/output_format.cc | Implements UTF-8 sanitization and routes CSV/JSON/table escaping through it. |
| cpp/tools/commands/cmd_sketch.cc | Sanitizes sketch output before printing/writing. |
| cpp/tools/commands/cmd_export.cc | Removes local JSON escaping and uses shared formatter utilities for manifests. |
| cpp/tools/README.md | Documents UTF-8 replacement behavior for CLI text outputs and manifests. |
| cpp/test/tools/output_format_test.cc | Adds unit tests for valid/invalid UTF-8 handling across formatters and RowWriter. |
| cpp/test/tools/command_e2e_test.cc | Adds end-to-end coverage ensuring invalid identifiers remain queryable but outputs are valid UTF-8. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
587
to
+600
| int code = printer.run(args, content, err); | ||
| const std::string text = replace_invalid_utf8(content.str()); | ||
| if (code != kExitOk) { | ||
| if (args.output.empty()) { | ||
| out << content.str(); | ||
| out << text; | ||
| out.flush(); | ||
| return out.good() ? code : kExitRuntime; | ||
| } | ||
| return code; | ||
| } | ||
| if (!args.output.empty()) { | ||
| return write_atomic_text(args.output, content.str(), args.file, | ||
| args.force, err); | ||
| return write_atomic_text(args.output, text, args.file, args.force, err); | ||
| } | ||
| out << content.str(); | ||
| out << text; |
| std::string out; | ||
| out.reserve(s.size() + 2); | ||
| for (unsigned char c : s) { | ||
| for (unsigned char c : replace_invalid_utf8(s)) { |
| std::string out; | ||
| out.reserve(s.size() + 2); | ||
| for (unsigned char c : s) { | ||
| for (unsigned char c : replace_invalid_utf8(s)) { |
| if (consumed == length) { | ||
| out.append(s, i, consumed); | ||
| } else { | ||
| out += "\xef\xbf\xbd"; |
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
October 8, 2026 09:47
Contributor
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
8 open findings
key, which is JSON-escaped (may include backslashes/escape… · Newemit_result_setcan return early on header validation failure without updatingemitted_rows. If… · Newjson_escape()andtable_escape()currently allocate an intermediate sanitized string via…json_escape()andtable_escape()currently allocate an intermediate sanitized string via…textis computed unconditionally (includingcontent.str()allocation + UTF-8 scan), but when…out += ...is typically okay, butpush_backcommunicates… · Newstd::set, which incurs O(n log n) inserts and extra… · New
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| if (!keys.insert(key).second) { | ||
| error_ = | ||
| "duplicate NDJSON column name after UTF-8 replacement: \"" + | ||
| key + "\""; |
Comment on lines
92
to
+98
| RowWriter writer(out, fmt, header, types, no_header); | ||
| if (!writer.error().empty()) { | ||
| if (output_error != nullptr) { | ||
| *output_error = writer.error(); | ||
| } | ||
| return common::E_INVALID_ARG; | ||
| } |
Comment on lines
+249
to
+250
| for_each_utf8_byte( | ||
| s, [&out](unsigned char c) { out += static_cast<char>(c); }); |
| table_widths_(header_.size(), 0) { | ||
| if (!no_header_) { | ||
| if (fmt_ == OutputFormat::kJson) { | ||
| std::set<std::string> keys; |
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.
TsFile files written through the SDK can contain malformed UTF-8 identifiers. The CLI currently copies those bytes into CSV, NDJSON and table output, so a name such as
bad\xffproduces invalid text instead ofbad�(TsFile-162).Replace each maximal ill-formed UTF-8 subpart with U+FFFD at the text output boundary. Share the conversion across the three formatters, export manifests and sketch output. Preserve valid Unicode, raw identifiers used for SDK lookup, and BLOB hexadecimal output. The replacement follows Unicode section 3.9.6.
NDJSON validates the rendered column names before writing rows. If distinct identifiers such as
bad\xffandbad\xfeboth becomebad�, the command reports the duplicate key and exits with code 3 instead of producing JSON that loses a column when parsed. This also covers table statistics TAG keys. Failed exports leave no new target and preserve an existing target with--force.JSON and table escaping consume the shared UTF-8 recovery stream directly, avoiding an intermediate sanitized string. NDJSON keys are escaped once per result and reused across rows. Sketch skips text conversion when a failed command will not publish output. All changes retain C++11 compatibility.
Validation:
git diff --checkpassed.Built in Release with LZ4 enabled and other optional compression libraries, ANTLR4 and SIMD disabled.