Correct wcwidth computation for pretty outputs. #3257

amosbird · 2018-09-29T14:19:17Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

https://github.com/ridiculousfish/widecharwidth license is pretty clean.

As PrettyBlockOutputStream and VerticalRowOutputStream are consumed by human, readability excels performance.

Before,

After,

alexey-milovidov · 2018-09-29T16:36:37Z

Great, we always wanted it!

alexey-milovidov · 2018-09-29T16:37:18Z

dbms/src/Common/UTF8Helpers.cpp

+
+size_t computeWidth(const UInt8 * data, size_t size)
+{
+    std::wstring wstr = std::wstring_convert<std::codecvt_utf8<wchar_t>, wchar_t>{}.from_bytes(reinterpret_cast<const char *>(data), reinterpret_cast<const char *>(data + size));


en.cppreference.com states that codecvt_utf8 is deprecated in C++17. Why?

alexey-milovidov · 2018-09-29T16:42:04Z

dbms/src/Common/UTF8Helpers.cpp

+
+size_t computeWidth(const UInt8 * data, size_t size)
+{
+    std::wstring wstr = std::wstring_convert<std::codecvt_utf8<wchar_t>, wchar_t>{}.from_bytes(reinterpret_cast<const char *>(data), reinterpret_cast<const char *>(data + size));


I've read that it will throw on invalid UTF-8 sequence:
https://en.cppreference.com/w/cpp/locale/wstring_convert/from_bytes
It's better to use replacement character �
(as your terminal does when invalid sequence is received)

alexey-milovidov · 2018-09-29T16:42:45Z

dbms/src/Common/UTF8Helpers.h

@@ -72,6 +72,8 @@ inline size_t countCodePoints(const UInt8 * data, size_t size)
    return res;
 }

+size_t computeWidth(const UInt8 * data, size_t size);


Missing comment.

alexey-milovidov · 2018-09-29T16:44:06Z

dbms/src/Common/UTF8Helpers.cpp

+        auto w = widechar_wcwidth(c);
+        if (w == -2)
+            width += 1;
+        else if (w > 0)


No cases for another special values.

alexey-milovidov · 2018-09-29T16:44:33Z

dbms/src/Common/UTF8Helpers.cpp

+    for (wchar_t c : wstr)
+    {
+        auto w = widechar_wcwidth(c);
+        if (w == -2)


You can use enum values.

alexey-milovidov · 2018-09-29T16:45:34Z

libs/libwidechar_width/widechar_width.h

+};
+
+/* An inclusive range of characters. */
+struct widechar_range {


Great! This library is implemented exactly as I thought.

alexey-milovidov · 2018-09-29T16:48:46Z

libs/CMakeLists.txt

@@ -27,3 +27,4 @@ if (USE_INTERNAL_CONSISTENT_HASHING_LIBRARY)
    add_subdirectory (consistent-hashing)
 endif ()
 add_subdirectory (consistent-hashing-sumbur)
+add_subdirectory (libwidechar_width)


Missing LICENCE and README in the library directory.
It must mention the exact source (URL, commit) of the library.

alexey-milovidov

Ok, but minor changed required.

alexey-milovidov · 2018-09-29T16:54:31Z

Missing test.

Test should have full-width characters (Chinese), zero-width characters,
two variants of Russian letter ё:
ё - d191 in UTF-8
ё - d0b5cc88 in UTF-8
as long as some invalid UTF-8 sequence,
and a string with zero bytes in the middle.

amosbird · 2018-09-30T03:32:54Z

@alexey-milovidov almost done. I don't find anything related to client testing. What should one look like?

two variants of Russian letter ё:

Can you post a csv file with that letter? I'm not sure if I can generate it correctly.

zhang2014 · 2018-09-30T04:18:19Z

@amosbird FYI, I hope that'll be helpful : )

amosbird · 2018-09-30T06:55:23Z

@zhang2014 Thanks. Those ёs are all of width 1 from what I can tell. I assume there is a variant that need special treatment.

amosbird · 2018-09-30T08:36:59Z

hmm, simple copy paste of that ё just works. Looks a bit weird in terminal though

alexey-milovidov · 2018-10-01T01:09:19Z

dbms/src/Common/UTF8Helpers.cpp

+namespace UTF8
+{
+
+// based on https://bjoern.hoehrmann.de/utf-8/decoder/dfa/


Could you please copy the license from this page verbatim here in comment?

alexey-milovidov · 2018-10-01T01:10:12Z

dbms/src/Common/UTF8Helpers.cpp

+    int width = widechar_wcwidth(wc);
+    switch (width)
+    {
+        case widechar_nonprint:


Add [[fallthrough]];
We have warnings in some compilers.

alexey-milovidov · 2018-10-01T01:10:58Z

dbms/src/Common/UTF8Helpers.cpp

+                // special treatment for '\t'
+                if (decoder.codepoint == '\t')
+                {
+                    do


Rewrite with simple formula?

alexey-milovidov · 2018-10-01T01:11:35Z

dbms/src/Common/UTF8Helpers.cpp

+                break;
+            // continue if we meet other values here
+            default:
+                rollback++;


Should use prefix increment according to style guide.

alexey-milovidov · 2018-10-01T01:12:18Z

dbms/src/Common/UTF8Helpers.h

@@ -72,6 +72,9 @@ inline size_t countCodePoints(const UInt8 * data, size_t size)
    return res;
 }

+/// returns UTF-8 wcswidth. Invalid sequence is treated as zero width character


Comment should decribe "prefix" parameter because its usage is non-obvious.

alexey-milovidov · 2018-10-01T18:38:13Z

And finally... look for the test
00298_enum_width_and_cast
https://travis-ci.org/yandex/ClickHouse/builds/435426678#L8971

amosbird · 2018-10-02T02:07:18Z

OK, updated 298's result

amosbird force-pushed the master branch from 5329769 to a170513 Compare September 29, 2018 14:58

alexey-milovidov reviewed Sep 29, 2018

View reviewed changes

alexey-milovidov requested changes Sep 29, 2018

View reviewed changes

amosbird force-pushed the master branch from a170513 to 9e751b0 Compare September 30, 2018 03:31

amosbird force-pushed the master branch from 9e751b0 to 2e595d1 Compare September 30, 2018 03:35

amosbird force-pushed the master branch 2 times, most recently from 43d04e2 to b9f1d06 Compare September 30, 2018 08:33

amosbird force-pushed the master branch from b9f1d06 to e675f8a Compare September 30, 2018 08:37

alexey-milovidov reviewed Oct 1, 2018

View reviewed changes

amosbird force-pushed the master branch 2 times, most recently from a63a87f to 9642496 Compare October 1, 2018 03:35

Correct wcwidth computation for pretty outputs.

7df7308

amosbird force-pushed the master branch from 9642496 to 7df7308 Compare October 2, 2018 02:06

filimonov mentioned this pull request Oct 2, 2018

Japanese characters have different width in cli output #888

Closed

alexey-milovidov merged commit 147a2a1 into ClickHouse:master Oct 2, 2018

alexey-milovidov added a commit that referenced this pull request Oct 9, 2018

Fixed issue in test #3257

89f3fd0

alexey-milovidov added a commit that referenced this pull request Oct 9, 2018

Fixed test (now letter ё is really decomposed) #3257

94d8a78

alexey-milovidov added a commit that referenced this pull request Oct 9, 2018

Updated test result #3257

6ca67ce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct wcwidth computation for pretty outputs. #3257

Correct wcwidth computation for pretty outputs. #3257

amosbird commented Sep 29, 2018 •

edited

Loading

alexey-milovidov commented Sep 29, 2018

alexey-milovidov Sep 29, 2018

alexey-milovidov Sep 29, 2018 •

edited

Loading

alexey-milovidov Sep 29, 2018

alexey-milovidov Sep 29, 2018

alexey-milovidov Sep 29, 2018

alexey-milovidov Sep 29, 2018

alexey-milovidov Sep 29, 2018 •

edited

Loading

alexey-milovidov left a comment

alexey-milovidov commented Sep 29, 2018 •

edited

Loading

amosbird commented Sep 30, 2018

zhang2014 commented Sep 30, 2018

amosbird commented Sep 30, 2018

amosbird commented Sep 30, 2018

alexey-milovidov Oct 1, 2018

amosbird Oct 1, 2018

alexey-milovidov Oct 1, 2018

alexey-milovidov Oct 1, 2018

alexey-milovidov Oct 1, 2018

alexey-milovidov Oct 1, 2018

alexey-milovidov commented Oct 1, 2018

amosbird commented Oct 2, 2018

Correct wcwidth computation for pretty outputs. #3257

Correct wcwidth computation for pretty outputs. #3257

Conversation

amosbird commented Sep 29, 2018 • edited Loading

alexey-milovidov commented Sep 29, 2018

Choose a reason for hiding this comment

alexey-milovidov Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

alexey-milovidov left a comment

Choose a reason for hiding this comment

alexey-milovidov commented Sep 29, 2018 • edited Loading

amosbird commented Sep 30, 2018

zhang2014 commented Sep 30, 2018

amosbird commented Sep 30, 2018

amosbird commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov commented Oct 1, 2018

amosbird commented Oct 2, 2018

amosbird commented Sep 29, 2018 •

edited

Loading

alexey-milovidov Sep 29, 2018 •

edited

Loading

alexey-milovidov Sep 29, 2018 •

edited

Loading

alexey-milovidov commented Sep 29, 2018 •

edited

Loading