Skip to content

Render tabs as 4 spaces when pasting into clickhouse-client#100416

Merged
Algunenano merged 3 commits intoClickHouse:masterfrom
Algunenano:tab_render
Mar 31, 2026
Merged

Render tabs as 4 spaces when pasting into clickhouse-client#100416
Algunenano merged 3 commits intoClickHouse:masterfrom
Algunenano:tab_render

Conversation

@Algunenano
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Improvement

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

Render tabs as 4 spaces when pasting into clickhouse-client. Closes #100405

Needs ClickHouse/replxx#37 merged first and then updating submodule

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 23, 2026

Workflow [PR], commit [4654f7f]

Summary:

job_name test_name status info comment
Stateless tests (amd_tsan, parallel, 2/2) failure
02889_file_log_save_errors FAIL cidb, issue ISSUE CREATED
Stress test (amd_asan_ubsan) failure
Unknown error FAIL cidb, issue ISSUE CREATED
Stress test (amd_msan) failure
Hung check failed, possible deadlock found FAIL cidb IGNORED
Stress test (arm_msan) failure
Logical error: Can't adjust last granule because it has A rows, but try to subtract B rows (num_read_rows = C, total_rows_per_granule = D, rows_per_granule = [E], debug: max_rows=F, rows_from_read=G, rows_from_finalize_loop=H, rows_from_finalize_post=I, ranges_processed=J, skipped_marks=K, use_query_condition_cache=L, can_read_incomplete_granules=M) (STID: 5258-4a6f) FAIL cidb, issue ISSUE CREATED
Stress test (arm_tsan) error IGNORED

AI Review

Summary

This PR updates contrib/replxx to render pasted tab characters as four spaces in interactive display while preserving literal tab bytes in the submitted query, and adds regression coverage in stateless tests (including an interactive expect scenario for bracketed paste mode). I did not find correctness, safety, concurrency, or performance issues in the introduced changes, and the PR metadata/changelog category appear aligned with the change.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR. labels Mar 23, 2026
Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but, there is one problem with it, if I will paste a query, the copy it back, I will have spaces not tabs

And the problem is only with copying what replxx renders I guess, but not with formatted query that is printed by ClickHouse AFAU

What do you think about this?

@@ -0,0 +1,2 @@
-- https://github.com/ClickHouse/ClickHouse/pull/84605
SELECT position('a a', '\t') > 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this has been broken in #84412?
It should not use replxx at all AFAIU

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it modified the query itself while pasting it, so ClickHouse got the modified query replacing any tab char with a space.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still not clear, why non-interactive mode uses replxx.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in CI with clickhouse-client --queries this will not fail. I just took the failed query from the revert to add it as part of the PR just in case

@Algunenano
Copy link
Copy Markdown
Member Author

Algunenano commented Mar 27, 2026

And the problem is only with copying what replxx renders I guess, but not with formatted query that is printed by ClickHouse AFAU

I'll see if I can improve it, but the current status is much worse since replxx gives you a broken query:

~ $ clickhouse local
ClickHouse local version 26.4.1.1.

:) select
^I1,
^I2
from my_table

SELECT
    1,
    2
FROM my_table

Query id: ad9db4ad-4c7c-44fc-ab6b-7f081621b1cc


Elapsed: 0.049 sec. 

Received exception:
Code: 60. DB::Exception: Unknown table expression identifier 'my_table' in scope SELECT 1, 2 FROM my_table. (UNKNOWN_TABLE)

:) select
^I1,
^I2
from my_table

Syntax error: failed at position 8 (^) (line 2, col 1):

select
^I1,
^I2
from my_table

Expected one of: ALL, DISTINCT ON, DISTINCT, TOP, not empty list of expressions, list of expressions, list of elements, expression with optional alias, element of expression with optional alias, lambda expression, CAST operator, NOT, INTERVAL, CASE, DATE, TIMESTAMP, tuple, collection of literals, array, number, literal, NULL, NULL, Bool, TRUE, FALSE, string literal, asterisk, qualified asterisk, compound identifier, identifier, COLUMNS matcher, COLUMNS, qualified COLUMNS matcher, function name, substitution, MySQL-style global variable

@azat
Copy link
Copy Markdown
Member

azat commented Mar 27, 2026

I'll see if I can improve it, but the current status is much worse since replxx gives you a broken query:

I see, then I guess we are good

@Algunenano
Copy link
Copy Markdown
Member Author

I can force replxx to emit a tab character but it has 2 drawbacks:

  • You need to update the cursor. I can make it 8 characters, which is the posix default, but if somebody changed it then it might cause issues.
  • Even when you then copy the output from replxx and try again, at least in my terminal (konsole), the paste buffer already replaces tabs and you get spaces, so it's complicating things unnecessarily.

I'll merge this as is. I don't think it's worth the headache

@Algunenano Algunenano added this pull request to the merge queue Mar 31, 2026
Merged via the queue into ClickHouse:master with commit 33ac0a0 Mar 31, 2026
158 of 164 checks passed
@Algunenano Algunenano deleted the tab_render branch March 31, 2026 09:47
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better support for hard tabs when pasting sql into clickhouse-client

3 participants