ClickHouse#81794 Pretty print JSON columns in Vertical format

[FRosner]

Changelog category (leave one):

• Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

JSON columns are now pretty printed when using Vertical format. Closes #81794.

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

Test Execution

foo@bar:~/projects/ClickHouse$ PATH=~/projects/ClickHouse/build/programs:$PATH tests/clickhouse-test 03700_vertical_format_pretty_print_json
Using queries from '/Users/frosner/projects/ClickHouse/tests/queries' directory
Connecting to ClickHouse server... OK
Connected to server 25.10.1.1 @ cab1c8b9ff3b357d4b922f5930b5c746829d53e4 frosner/81794
Found 1 parallel tests and 0 sequential tests
Running about 1 stateless tests (Process-3).
03700_vertical_format_pretty_print_json:                                [ OK ] 0.30 sec.

1 tests passed. 0 tests skipped. 0.34 s elapsed (Process-3).                                                                                                                                      
All tests have finished.

[CLAassistant]

All committers have signed the CLA.

[clickhouse-gh]

Workflow [PR], commit [95c2885]

Summary: ❌

job_name	test_name	status	info	comment
Integration tests (arm_binary, distributed plan, 1/4)		failure
	test_keeper_three_nodes_two_alive/test.py::test_restart_third_node	FAIL	cidb

[alexey-milovidov]

charset is not the best switch for this. It should be controlled by a new setting in FormatFactorySettings.

[alexey-milovidov]

This looks strange anyway. Shouldn't it be a separate method, SerializationNullable::serializeTextJSONPretty?

[FRosner]

charset is not the best switch for this. It should be controlled by a new setting in FormatFactorySettings.

Yeah I copied that from

ClickHouse/src/DataTypes/Serializations/SerializationNullable.cpp

Line 802 in 9e89905

if (settings.pretty.charset == FormatSettings::Pretty::Charset::UTF8)

and it appears to be used in other places as well. We could introduce a new property in this PR if you want?

This looks strange anyway. Shouldn't it be a separate method, SerializationNullable::serializeTextJSONPretty?

Yeah, let's do that. It was pretty much duplicated but it's more consistent.

Here it goes: d462932

[alexey-milovidov]

The change is generally good!

[alexey-milovidov]

Interesting why do we print an empty line?

[alexey-milovidov]

Or pass the indent that will make it roughly aligned?

[FRosner]

Interesting why do we print an empty line?

ClickHouse/src/DataTypes/Serializations/SerializationJSON.cpp

Lines 147 to 150 in 9e89905

	if (pretty)
	writeCString("{\n", ostr);
	else
	writeChar('{', ostr);

ClickHouse/src/DataTypes/Serializations/SerializationJSON.cpp

Lines 283 to 293 in 9e89905

	if (pretty)
	{
	writeChar('\n', ostr);
	for (size_t i = 0; i != current_prefix.elements.size(); ++i)
	{
	writeChar(settings.json.pretty_print_indent, (indent + current_prefix.size() - i) * settings.json.pretty_print_indent_multiplier, ostr);
	writeCString("}\n", ostr);
	}
	writeChar(settings.json.pretty_print_indent, indent * settings.json.pretty_print_indent_multiplier, ostr);
	writeChar('}', ostr);
	}

Do you want me to change that here, too? It might be a bit out of scope for this LHF issue, wdyt?

[alexey-milovidov]

That's ok, let's keep it as is.

[FRosner]

There are some failing tests that seem unrelated? Are those expeted to be green?

[darkleaf]

@FRosner this breaks our CI. Could you please add a setting for the feature? Then we could use SETTINGS compatibility='25.3' to preserve old behavior while we support 25.3.

[FRosner]

Could you please add a setting for the feature? Then we could use SETTINGS compatibility='25.3' to preserve old behavior while we support 25.3.

Oops! Sorry. Which CI is broken how? Can you elaborate?

[KGOH]

@FRosner (I'll answer for @darkleaf): our regression tests assert output in Vertical format, which supposedly after this PR includes pretty printed jsons
Since our tests run at multiple clickhouse versions, e.g. at 25.3 and at latest, now with the newer latest this makes the results to be different on different versions which fails our assertions on CI:

[darkleaf]

We test our product against 25.3 and latest.
And we use snapshots to test our inserts.
The snapshot is a text file produced by select ... from ... format Vertical
We expect that this snapshots are equal for all supported versions.

[FRosner]

Thanks for the context. I'm not the one to decide how to handle this (this is my first contribution and I'm not working for ClickHouse at this point), so I'll let @alexey-milovidov or someone else decide.

If anyone wants my 2 cents, though: Parsing an output format that is documented to be "only appropriate for outputting a query result, but not for parsing" might not be ideal. Why are you relying on vertical output format? Can't you use another format that is meant to be machine-readable?

[darkleaf]

Why are you relying on vertical output format? Can't you use another format that is meant to be machine-readable?

Our team members read this files on code review, so it should be human readable. We don't parse, we only diff it.

I’d be glad to see a setting similar to these.

[alexey-milovidov]

I'd prefer if @darkleaf would introduce the required setting in a separate pull request.