GH-50830: [C++][Parquet] Use JsonWriter for LogicalType::ToJSON()

[NathanChung4]

Rationale for this change

This change was made because it refactored ToJSON() to build JSON through a proper writer instead of hand-built strings, which removes a correctness risk. It also aligns the codebase's ongoing transition away from manual JSON construction.

What changes are included in this PR?

This PR focused on changing the 8 ToJSON() functions from hand built strings to using JsonWriter. In addition, the test files (schema_test.cc, reader_test.cc) were also updated to reflect these changes. Finally, WriteCrsKeyAndValue were no longer needed so it was deleted.

Are these changes tested?

These changes are tested and verified. There was a parquet-schema-test in which 41/41 testcases passed. There was a parquet-reader-test in which 149/154 passed and 5 were skipped as they were unrelated. A Full ctest -R "^parquet-": 11/11 suites passed. And finally the pre-commit (C++ Format + C++ Lint) came out clean.

Are there any user-facing changes?

Yes, there are user-facing changes. ToJSON() now produces compact JSON instead of spaced JSON. It's the same valid JSON, but different exact bytes:

• Before: {"Type": "Decimal", "precision": 10, "scale": 4}
• After: {"Type":"Decimal","precision":10,"scale":4}

AI Disclosure

Per the AI-generated code guidance: the 8 ToJSON() transitions to JsonWriter, test file changes, the merge conflict resolutions, and the test were produced with Claude Code, and reviewed and verified by me. Correctness was checked by rebuilding and running the relevant test after every single function conversion, not just once at the end. In addition, catching two mistakes early via failing tests (the Decimal attempt that dropped fields, the Int attempt that did the same).

• GitHub Issue: [C++] Use JsonWriter consistently in LogicalType::ToJSON() #50830

[Copilot]

Pull request overview

Refactors Parquet C++ LogicalType::ToJSON() implementations to consistently use ::arrow::json::JsonWriter rather than manual string/stream construction, resulting in compact JSON output and reduced risk of producing invalid JSON for unescaped values.

Changes:

• Replaced hand-built JSON in 8 LogicalType::Impl::*::ToJSON() implementations with JsonWriter-based construction.
• Removed the now-unneeded WriteCrsKeyAndValue helper and simplified CRS emission to StringField.
• Updated Parquet schema and reader tests to match the new compact JSON formatting.

Reviewed changes

Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.

File	Description
cpp/src/parquet/types.cc	Converts LogicalType::ToJSON() methods to JsonWriter and removes manual/stream JSON helper code.
cpp/src/parquet/schema_test.cc	Updates expected ToJSON() strings for logical types to match compact JSON output.
cpp/src/parquet/reader_test.cc	Updates expected JSON substrings containing embedded logical-type JSON to match compact formatting.

---

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

[Reranko05]

Could we add an IntField() helper to JsonWriter and use it here, similar to StringField() and BoolField()? It would make these conversions a bit cleaner.

[NathanChung4]

Hey Reranko05, thank you for the suggestion and I totally agree with you. I have committed the proposed change, please let me know if you want to change anything or anything

[Copilot]

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated no new comments.

[Reranko05]

LGTM

[Reranko05]

@kou Can you review this when you have time?

[kou]

Could you use JsonWriter in

arrow/cpp/src/parquet/printer.cc

Lines 254 to 435 in f64e90a

	void ParquetFilePrinter::JSONPrint(std::ostream& stream, std::list<int> selected_columns,
	const char* filename) {
	const FileMetaData* file_metadata = fileReader->metadata().get();
	stream << "{\n";
	stream << " \"FileName\": \"" << filename << "\",\n";
	stream << " \"Version\": \"" << ParquetVersionToString(file_metadata->version())
	<< "\",\n";
	stream << " \"CreatedBy\": \"" << file_metadata->created_by() << "\",\n";
	stream << " \"TotalRows\": \"" << file_metadata->num_rows() << "\",\n";
	stream << " \"NumberOfRowGroups\": \"" << file_metadata->num_row_groups() << "\",\n";
	stream << " \"NumberOfRealColumns\": \""
	<< file_metadata->schema()->group_node()->field_count() << "\",\n";
	stream << " \"NumberOfColumns\": \"" << file_metadata->num_columns() << "\",\n";
	if (selected_columns.empty()) {
	for (int i = 0; i < file_metadata->num_columns(); i++) {
	selected_columns.push_back(i);
	}
	} else {
	for (auto i : selected_columns) {
	if (i < 0 || i >= file_metadata->num_columns()) {
	throw ParquetException("Selected column is out of range");
	}
	}
	}
	stream << " \"Columns\": [\n";
	int c = 0;
	for (auto i : selected_columns) {
	const ColumnDescriptor* descr = file_metadata->schema()->Column(i);
	stream << " { \"Id\": \"" << i << "\","
	<< " \"Name\": \"" << descr->path()->ToDotString() << "\","
	<< " \"PhysicalType\": \""
	<< TypeToString(descr->physical_type(), descr->type_length()) << "\","
	<< " \"ConvertedType\": \"" << ConvertedTypeToString(descr->converted_type())
	<< "\","
	<< " \"LogicalType\": " << (descr->logical_type())->ToJSON() << " }";
	c++;
	if (c != static_cast<int>(selected_columns.size())) {
	stream << ",\n";
	}
	}
	stream << "\n ],\n \"RowGroups\": [\n";
	for (int r = 0; r < file_metadata->num_row_groups(); ++r) {
	stream << " {\n \"Id\": \"" << r << "\", ";
	auto group_reader = fileReader->RowGroup(r);
	std::unique_ptr<RowGroupMetaData> group_metadata = file_metadata->RowGroup(r);
	stream << " \"TotalBytes\": \"" << group_metadata->total_byte_size() << "\", ";
	stream << " \"TotalCompressedBytes\": \"" << group_metadata->total_compressed_size()
	<< "\", ";
	auto row_group_sorting_columns = group_metadata->sorting_columns();
	if (!row_group_sorting_columns.empty()) {
	stream << " \"SortColumns\": [\n";
	for (size_t i = 0; i < row_group_sorting_columns.size(); i++) {
	stream << " {\"column_idx\": " << row_group_sorting_columns[i].column_idx
	<< ", \"descending\": " << row_group_sorting_columns[i].descending
	<< ", \"nulls_first\": " << row_group_sorting_columns[i].nulls_first
	<< "}";
	if (i + 1 != row_group_sorting_columns.size()) {
	stream << ",";
	}
	stream << '\n';
	}
	stream << " ], ";
	}
	stream << " \"Rows\": \"" << group_metadata->num_rows() << "\",\n";
	// Print column metadata
	stream << " \"ColumnChunks\": [\n";
	int c1 = 0;
	for (auto i : selected_columns) {
	auto column_chunk = group_metadata->ColumnChunk(i);
	std::shared_ptr<Statistics> stats = column_chunk->statistics();
	const ColumnDescriptor* descr = file_metadata->schema()->Column(i);
	stream << " {\"Id\": \"" << i << "\", \"Values\": \""
	<< column_chunk->num_values() << "\", "
	<< "\"StatsSet\": ";
	if (column_chunk->is_stats_set()) {
	stream << R"("True", "Stats": {)";
	if (stats->HasNullCount()) {
	stream << R"("NumNulls": ")" << stats->null_count() << "\"";
	}
	if (stats->HasDistinctCount()) {
	stream << ", "
	<< R"("DistinctValues": ")" << stats->distinct_count() << "\"";
	}
	if (stats->HasMinMax()) {
	std::string min = stats->EncodeMin(), max = stats->EncodeMax();
	stream << ", "
	<< R"("Max": ")"
	<< FormatStatValue(descr->physical_type(), max, descr->logical_type())
	<< "\", "
	<< R"("Min": ")"
	<< FormatStatValue(descr->physical_type(), min, descr->logical_type())
	<< "\"";
	if (stats->is_max_value_exact().has_value()) {
	stream << ", "
	<< R"("IsMaxValueExact": ")"
	<< (stats->is_max_value_exact().value() ? "True" : "False") << "\"";
	} else {
	stream << ", "
	<< R"("IsMaxValueExact": "unknown")";
	}
	if (stats->is_min_value_exact().has_value()) {
	stream << ", "
	<< R"("IsMinValueExact": ")"
	<< (stats->is_min_value_exact().value() ? "True" : "False") << "\"";
	} else {
	stream << ", "
	<< R"("IsMinValueExact": "unknown")";
	}
	}
	stream << " },";
	} else {
	stream << "\"False\",";
	}
	stream << "\n \"Compression\": \""
	<< ::arrow::internal::AsciiToUpper(
	Codec::GetCodecAsString(column_chunk->compression()))
	<< R"(", "Encodings": )";
	stream << "\"";
	if (column_chunk->encoding_stats().empty()) {
	for (auto encoding : column_chunk->encodings()) {
	stream << EncodingToString(encoding) << " ";
	}
	} else {
	PrintPageEncodingStats(stream, column_chunk->encoding_stats());
	}
	stream << "\"";
	stream << ", "
	<< R"("UncompressedSize": ")" << column_chunk->total_uncompressed_size()
	<< R"(", "CompressedSize": ")" << column_chunk->total_compressed_size()
	<< "\"";
	if (column_chunk->bloom_filter_offset()) {
	// Output BloomFilter {offset, length}
	stream << ", \"BloomFilter\": {"
	<< R"("offset": ")" << column_chunk->bloom_filter_offset().value() << "\"";
	if (column_chunk->bloom_filter_length()) {
	stream << R"(, "length": ")" << column_chunk->bloom_filter_length().value()
	<< "\"";
	}
	stream << "}";
	}
	if (column_chunk->GetColumnIndexLocation()) {
	auto location = column_chunk->GetColumnIndexLocation().value();
	// Output ColumnIndex {offset, length}
	stream << ", \"ColumnIndex\": {"
	<< R"("offset": ")" << location.offset;
	stream << R"(", "length": ")" << location.length;
	stream << "\"}";
	}
	if (column_chunk->GetOffsetIndexLocation()) {
	auto location = column_chunk->GetOffsetIndexLocation().value();
	// Output OffsetIndex {offset, length}
	stream << ", \"OffsetIndex\": {"
	<< R"("offset": ")" << location.offset << "\"";
	stream << R"(, "length": ")" << location.length << "\"";
	stream << "}";
	}
	// end of a ColumnChunk
	stream << " }";
	c1++;
	if (c1 != static_cast<int>(selected_columns.size())) {
	stream << ",\n";
	}
	}
	stream << "\n ]\n }";
	if ((r + 1) != static_cast<int>(file_metadata->num_row_groups())) {
	stream << ",\n";
	}
	}
	stream << "\n ]\n}\n";
	}

too?

It seems that we can use https://github.com/simdjson/simdjson/blob/master/doc/builder.md#pretty-formatted-fractured-json for it.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

cpp/src/parquet/reader_test.cc:1169

• These tests now assert exact pretty-printed JSON bytes. Since the formatting is produced by simdjson::fractured_json_string (and may change across simdjson updates), the tests can become brittle even if the JSON content is semantically unchanged. Consider parsing + asserting key fields, or normalizing both strings (e.g., minify/canonicalize) before comparison.

  std::string json_content = ReadFromLocalFile("nested_lists.snappy.parquet");
  ASSERT_EQ(json_output, json_content);
}

cpp/src/parquet/printer.cc:429

• JSONPrint now builds JSON and then reparses it via GetPrettyString() to pretty-print. For large file metadata this adds a full extra parse + allocation pass; consider either (a) emitting compact JSON via GetString() here, or (b) making pretty-print optional (flag/arg) so callers can avoid the extra work when not needed.

  writer.EndObject();

  PARQUET_ASSIGN_OR_THROW(std::string pretty_json, writer.GetPrettyString());
  stream << pretty_json << "\n";

cpp/src/arrow/json/json_writer_internal.h:66

• New JsonWriter APIs (IntField / GetPrettyString) are now relied on by Parquet code, but there are no unit tests covering their behavior. Adding small gtest coverage would help catch formatting or type-regression issues early (especially since GetPrettyString depends on simdjson pretty-print behavior).

  void StringField(std::string_view key, std::string_view value);
  void BoolField(std::string_view key, bool value);
  void IntField(std::string_view key, int32_t value);

  Result<std::string_view> GetString() const;

  Result<std::string> GetPrettyString(
      const simdjson::fractured_json_options& options = {}) const;

[Copilot]

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

cpp/src/parquet/printer.cc:270

• JSONPrint() now builds the entire JSON document in-memory and then pretty-prints it (GetPrettyString()), which duplicates the JSON (builder buffer + pretty string) before writing to stream. For large files with many row groups/columns, this is a potentially significant memory regression compared to the previous streaming implementation.

Consider making pretty-printing optional (defaulting to compact GetString()), or preserving a streaming approach for large outputs.

void ParquetFilePrinter::JSONPrint(std::ostream& stream, std::list<int> selected_columns,
                                   const char* filename) {
  const FileMetaData* file_metadata = fileReader->metadata().get();
  ::arrow::json::JsonWriter writer;
  writer.StartObject();
  writer.StringField("FileName", filename);
  writer.StringField("Version", ParquetVersionToString(file_metadata->version()));
  writer.StringField("CreatedBy", file_metadata->created_by());
  writer.StringField("TotalRows", std::to_string(file_metadata->num_rows()));
  writer.StringField("NumberOfRowGroups",
                     std::to_string(file_metadata->num_row_groups()));
  writer.StringField(
      "NumberOfRealColumns",
      std::to_string(file_metadata->schema()->group_node()->field_count()));
  writer.StringField("NumberOfColumns", std::to_string(file_metadata->num_columns()));

cpp/src/parquet/reader_test.cc:1192

• JSONOutput compares the entire pretty-printed JSON string, but the expected Columns entries here look inconsistent with how ParquetFilePrinter::JSONPrint() now writes fields (writer order is Id/Name/PhysicalType/ConvertedType/LogicalType, and GetPrettyString() formatting is unlikely to emit the aligned spacing + reordered keys shown). This is likely to make the test fail and/or be very brittle across simdjson pretty-printer changes.

Consider either regenerating the golden string from the new output, or (preferably) parsing json_content and asserting on the relevant fields/values instead of full-string equality.

  std::string json_output = R"###({
    "FileName": "alltypes_plain.parquet",
    "Version": "1.0",
    "CreatedBy": "impala version 1.3.0-INTERNAL (build 8a48ddb1eff84592b3fc06bc6f51ec120e1fffc9)",
    "TotalRows": "8",
    "NumberOfRowGroups": "1",
    "NumberOfRealColumns": "11",
    "NumberOfColumns": "11",
    "Columns": [
        { "ConvertedType": "NONE", "Id": "0" , "LogicalType": { "Type": "None" }, "Name": "id"             , "PhysicalType": "INT32" },
        { "ConvertedType": "NONE", "Id": "1" , "LogicalType": { "Type": "None" }, "Name": "bool_col"       , "PhysicalType": "BOOLEAN" },
        { "ConvertedType": "NONE", "Id": "2" , "LogicalType": { "Type": "None" }, "Name": "tinyint_col"    , "PhysicalType": "INT32" },
        { "ConvertedType": "NONE", "Id": "3" , "LogicalType": { "Type": "None" }, "Name": "smallint_col"   , "PhysicalType": "INT32" },
        { "ConvertedType": "NONE", "Id": "4" , "LogicalType": { "Type": "None" }, "Name": "int_col"        , "PhysicalType": "INT32" },
        { "ConvertedType": "NONE", "Id": "5" , "LogicalType": { "Type": "None" }, "Name": "bigint_col"     , "PhysicalType": "INT64" },
        { "ConvertedType": "NONE", "Id": "6" , "LogicalType": { "Type": "None" }, "Name": "float_col"      , "PhysicalType": "FLOAT" },
        { "ConvertedType": "NONE", "Id": "7" , "LogicalType": { "Type": "None" }, "Name": "double_col"     , "PhysicalType": "DOUBLE" },
        { "ConvertedType": "NONE", "Id": "8" , "LogicalType": { "Type": "None" }, "Name": "date_string_col", "PhysicalType": "BYTE_ARRAY" },
        { "ConvertedType": "NONE", "Id": "9" , "LogicalType": { "Type": "None" }, "Name": "string_col"     , "PhysicalType": "BYTE_ARRAY" },
        { "ConvertedType": "NONE", "Id": "10", "LogicalType": { "Type": "None" }, "Name": "timestamp_col"  , "PhysicalType": "INT96" }
    ],