58363 Automatically disable ANSI escape sequences in Pretty formats if the output is not a terminal

[Blargian]

Changelog category (leave one):

• New Feature

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

Automatically disable ANSI escape sequences in Pretty formats if the output is not a terminal. Add new auto mode to setting output_format_pretty_color.

Hi,

I'm opening a draft pull request for #58363 (hope that's okay!). I implemented the following changes so far together with a stateless test to check the functionality.

Changes made:

• modified output_format_pretty_color to use options Off, On, Auto (Core/Settings.h and FormatFactory.cpp)
• Added an enum class PrettyColor with these options (FormatSettings.h)
• Modified PrettyBlockOutputFormat.h to switch on the settings above and turn pretty.color on or off based on conditions provided in the issue.
• Added 00405_output_format_pretty_color.sql and generated the reference file.

Help points:

• The behaviour appears correct to me for On and Off but I don't believe I have the correct usage of isatty function for the Auto setting. I'm unsure on this point.
• I went with On,Off and Auto but you did mention that you want 0,1 and auto as the setting options. Could I achieve this somehow with the enum class or is there a better alternative?
• Any comments on formatting / performance
• Manually testing from http://localhost:8123/play the behaviour is not correct. It is different to the reference file, so perhaps it is user trouble.

[robot-clickhouse-ci-1]

This is an automated comment for commit a457fa3 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs Check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	✅ success

Check name	Description	Status
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	❌ failure
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	❌ failure

[Avogar]

I went with On,Off and Auto but you did mention that you want 0,1 and auto as the setting options. Could I achieve this somehow with the enum class or is there a better alternative?

You can use special struct-wrapper UInt64Auto for it. See as an example setting insert_quorum:

ClickHouse/src/Core/Settings.h

Line 267 in dbb19e7

M(UInt64Auto, insert_quorum, 0, "For INSERT queries in the replicated table, wait writing for the specified number of replicas and linearize the addition of the data. 0 - disabled, 'auto' - use majority", 0) \

[Blargian]

Hi @Avogar, I've implemented the changes you mentioned in the latest commit.

• Using UInt64Auto now for output_format_pretty_color setting
• Added isWritingToTerminal to IO/WriteHelpers.h (I marked it inline otherwise we get a "multiple defined symbols linker error")
• Updated the test to reflect the changes and the reference file seems to display the behaviour we are looking for now

One thing which is still bothering me, is that if I manually test using http://localhost:8123/play and run

SET output_format_pretty_color = 0
SELECT number AS hello, toString(number) AS world, (hello, world) AS tuple, nullIf(hello % 3, 0) AS sometimes_nulls FROM system.numbers LIMIT 10 SETTINGS max_block_size = 5 FORMAT Pretty;

I get back an output with ANSI Escapes still but expect not to see any escapes. But maybe I'm just not changing the setting correctly because if I run afterwards query

SELECT * FROM system.settings WHERE name='output_format_pretty_color'

value is still auto

[Avogar]

No need to add a new setting for it. We can just change the type of color setting.

[Avogar]

We need to change documentation for this setting in docs/en/operations/settings/settings-formats.md

[Avogar]

One thing which is still bothering me, is that if I manually test using http://localhost:8123/play and run <...> I get back an output with ANSI Escapes still but expect not to see any escapes. But maybe I'm just not changing the setting correctly

AFAIK multi-statemens are not allowed in play UI, so we cannot do

set output_format_pretty_color =...;
select ... format Pretty;

And if you just first execute SET query and then SELECT, then setting change won't take any effect, because each HTTP query has it's own query context with it's own set of settings. To test it you can do:

select ... format Pretty settings output_format_pretty_color=...;

[Blargian]

We need to change documentation for this setting in docs/en/operations/settings/settings-formats.md

Are only En language docs kept up to date fully? I notice that Ru documentation contains only some of output_format_pretty settings. Можно добавить output_format_pretty_color к docs/ru/operations/settings/settings.md? ... на сломанном русском... грамматическое качество не гарантирую!

[Avogar]

Are only En language docs kept up to date fully?

Yes, ClickHouse team keeps English docs up to date, but other languages are supported as much as possible by external contributors from different countries.

Можно добавить output_format_pretty_color к docs/ru/operations/settings/settings.md? ... на сломанном русском... грамматическое качество не гарантирую!

Only if you want, it's not necessary. But if you do it, I will check the the grammar

[Blargian]

@Avogar I pushed the latest changes. When generating the reference file again on these changes ANSI escapes are no longer shown for auto when they should be. If I step through the code with lldb when outcome_format_pretty_color=auto the logic seems correct to me, I don't spot something unusual. Eg) for normal Pretty format color below evaluates as 1 and if I step through in PrettyBlockOutputFormat.cpp I can see that color is used correctly. Can you spot anything unusual in the changes I made that might cause the output to be incorrect?

bool color = !no_escapes && format_settings.pretty.color.valueOr(isWritingToTerminal(buf));
return std::make_shared<OutputFormat>(buf, sample, format_settings, mono_block, color);

[Avogar]

Ah, the problem is that Pretty format supports parallel formatting and it's enabled by default. During parallel formatting we output blocks of data in Pretty format in parallel in special memory chunks and then output these chunks to the initial write buffer in corresponding order. So when we create PrettyBlockOutputFormat we have WriteBuffer that writes to some memory, not to stdout, ParallelFormattingOutputFormat actually works with initial WriteBuffer to stdout.
Probably the best solution will be to add a new common field in FormatSettings with name is_writing_to_terminal and set it when we create output format in FormatFactory::getOutputFormat/getOutputFormatParallelIfPossible when we initially create format settings and have initial WriteBuffer.
Here:

ClickHouse/src/Formats/FormatFactory.cpp

Lines 491 to 492 in 43b3120

	auto format_settings = _format_settings ? *_format_settings : getFormatSettings(context);
	format_settings.max_threads = context->getSettingsRef().max_threads;

And here:

ClickHouse/src/Formats/FormatFactory.cpp

Line 521 in 43b3120

auto format_settings = _format_settings ? *_format_settings : getFormatSettings(context);

This new field is_writing_to_terminal will be just a common property that any format can use.

[Blargian]

@Avogar if I understood correctly, we will set the value of this common field is_writing_to_terminal using the helper function isWritingToTerminal(buf) previously added to IO/WriteHelpers? The idea being that we need to check if we are writing to a terminal before the creator function in PrettyBlockOutputFormat.h gets called? I.e before we get the format.

and yet another question. I'm trying to understand why I get a different result when running

clickhouse --client --multiquery < 00405_output_format_pretty_color.sql

than from when I store the output in the reference file with

clickhouse --client --multiquery < 00405_output_format_pretty_color.sql > 00405_output_format_pretty_color.reference

I would expect at very least to see the ANSI escapes with clickhouse --client --multiquery < 00405_output_format_pretty_color.sql as the result is printed in the terminal in which the command is run.

[Avogar]

if I understood correctly, we will set the value of this common field is_writing_to_terminal using the helper function isWritingToTerminal(buf) previously added to IO/WriteHelpers?

Yes.

The idea being that we need to check if we are writing to a terminal before the creator function in PrettyBlockOutputFormat.h gets called? I.e before we get the format

Yes, because in creator it's not guaranteed that the write buffer will write to the actual destination, but in FormatFactory::getOutputFormat/getOutputFormatParallelIfPossible it's guaranteed.

and yet another question. I'm trying to understand why I get a different result when running than from when I store the output in the reference file with

I see that the result is expected in both cases. In first case with output to the terminal we have bold column names constructed by escapes as expected. In the second case with output to file I see escape sequances in the result, the only difference that in the second case escape sequances are not displayed properly (probably you used less to see the file or smth similar).

[Avogar]

Many tests failed, but it's expected as we changed the value of the setting. We need to update these tests, either update reference files or set output_format_pretty_color=1 explicitly in these tests

[Avogar]

Let's add this change to SettingsChangesHistory.h

[Blargian]

@Avogar okay, clear. I was using vim to view the reference file which is why the escapes ^[[0m show up like that. Even so something is still wrong. In the reference file with output_format_pretty_color = auto and using FORMAT Pretty we see this:

┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     0 │ 0     │ (0,'0') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     1 │ 1     │ (1,'1') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     2 │ 2     │ (2,'2') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     3 │ 3     │ (3,'3') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     4 │ 4     │ (4,'4') │               1 │
└───────┴───────┴─────────┴─────────────────┘

and for FORMAT PrettyNoEscapes we see this:

┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     0 │ 0     │ (0,'0') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     1 │ 1     │ (1,'1') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     2 │ 2     │ (2,'2') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     3 │ 3     │ (3,'3') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     4 │ 4     │ (4,'4') │               1 │
└───────┴───────┴─────────┴─────────────────┘

However, we should actually see for FORMAT Pretty this, as we see when output_format_pretty_color = 1:

┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ �[1mhello�[0m ┃ �[1mworld�[0m ┃ �[1mtuple  �[0m ┃ �[1msometimes_nulls�[0m ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     0 │ 0     │ (0,'0') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     1 │ 1     │ (1,'1') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     2 │ 2     │ (2,'2') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     3 │ 3     │ (3,'3') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     4 │ 4     │ (4,'4') │               1 │
└───────┴───────┴─────────┴─────────────────┘

CI is also failing on Fast tests for tests which are using Pretty format.

What is confusing me is that if I step through the code for Pretty and PrettyNoEscapes, color is getting set as we would expect - true for Pretty but false for PrettyNoEscapes.

As you mentioned we should see the bold column names for Pretty however I get the exact same formatting output to the terminal for both Pretty and PrettyNoEscapes.

Running SELECT number AS hello, toString(number) AS world, (hello, world) AS tuple, nullIf(hello % 3, 0) AS sometimes_nulls FROM system.numbers LIMIT 10 SETTINGS max_block_size = 5 FORMAT Pretty;

┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     0 │ 0     │ (0,'0') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     1 │ 1     │ (1,'1') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     2 │ 2     │ (2,'2') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     3 │ 3     │ (3,'3') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     4 │ 4     │ (4,'4') │               1 │
└───────┴───────┴─────────┴─────────────────┘
┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     5 │ 5     │ (5,'5') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     6 │ 6     │ (6,'6') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     7 │ 7     │ (7,'7') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     8 │ 8     │ (8,'8') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     9 │ 9     │ (9,'9') │            ᴺᵁᴸᴸ │
└───────┴───────┴─────────┴─────────────────┘

and running SELECT number AS hello, toString(number) AS world, (hello, world) AS tuple, nullIf(hello % 3, 0) AS sometimes_nulls FROM system.numbers LIMIT 10 SETTINGS max_block_size = 5 FORMAT PrettyNoEscapes;

┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     0 │ 0     │ (0,'0') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     1 │ 1     │ (1,'1') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     2 │ 2     │ (2,'2') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     3 │ 3     │ (3,'3') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     4 │ 4     │ (4,'4') │               1 │
└───────┴───────┴─────────┴─────────────────┘
┏━━━━━━━┳━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃ hello ┃ world ┃ tuple   ┃ sometimes_nulls ┃
┡━━━━━━━╇━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│     5 │ 5     │ (5,'5') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     6 │ 6     │ (6,'6') │            ᴺᵁᴸᴸ │
├───────┼───────┼─────────┼─────────────────┤
│     7 │ 7     │ (7,'7') │               1 │
├───────┼───────┼─────────┼─────────────────┤
│     8 │ 8     │ (8,'8') │               2 │
├───────┼───────┼─────────┼─────────────────┤
│     9 │ 9     │ (9,'9') │            ᴺᵁᴸᴸ │
└───────┴───────┴─────────┴─────────────────┘

So this is the behaviour I observe even when I can see with lldb that for the first query color = true and in the second query color = false.

Possibly it's just me who is confused :D

[Avogar]

The tests are failing in the CI because when we generate reference files we redirect the output to the file, so don't write to the terminal and our auto mode disables escape sequences. And this is ok.
I just downloaded the binary from fasttest with your cahnges and when I run command clickhouse client -q "select 42 as value format Pretty" without redirection to the file, it prints escape sequences, when I run clickhouse client -q "select 42 as value format Pretty" > reference, then it doesn't print escape sequences. So, seems like everything is ok.

[Avogar]

We are almost ready to merge! Several tests need to be adjusted and I will approve and merge

[Blargian]

@Avogar sorry for the delay, I got stuck now on trying to fix the last remaining tests which are failing. I don't find any test test_parallel_replicas_custom_key_failover in my local repository, only test_parallel_replicas_custom_key, but I don't see anything related to Pretty format there. As for Flaky tests I don't even know where to begin to fix that...

[Avogar]

I got stuck now on trying to fix the last remaining tests which are failing

Actually, all failed tests are not related to the changes in this PR. test_parallel_replicas_custom_key_failover test is broken in master and our team is looking into it. Integration flaky check failed due to timeout and it's an infrastracture problem. Stateless tests flaky check failed because of test 01074_partial_revokes, but it's expected for this test, it should not be run in parallel (we need to add special tag to this test to avoid it).

[Blargian]

@Avogar Any other open issues around this part of the codebase for Pretty/Formats you can recommend for me to work on?

[Avogar]

#31363 - usability improvement for Template format. Will not be difficult, but really useful.
#56912 - usability improvement for Pretty format.
#56257 - usability improvement for TSV format, I wanted to do it but don't have time now. Should be not so difficult, but will require careful implementation.

If you are ready to do something more complex, you can try to implement a new format:
Npy output format - #56345
AvroConfluent output format - #41204

[alexey-milovidov]

Amazing! It works:

watch -n1 'clickhouse-local --query "SELECT now() FORMAT Pretty"'