Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON API based on simdjson #5124

Merged
merged 12 commits into from
May 8, 2019
Merged

Conversation

vitlibar
Copy link
Member

@vitlibar vitlibar commented Apr 26, 2019

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

Add functions to extract values from JSON.
This pull request contains commits from #4686 written by hczhcz.

Category (leave one):

  • New Feature

This PR adds several functions extracting values from JSON:

select jsonHas('{"a": "hello", "b": [-100, 200.0, 300]}', 'a') = 1
select jsonExtractString('{"a": "hello", "b": [-100, 200.0, 300]}', 1) = 'a'
select jsonExtractString('{"a": "hello", "b": [-100, 200.0, 300]}', 'a') = 'hello'
select jsonExtractInt('{"a": "hello", "b": [-100, 200.0, 300]}', 'b', 1) = -100
select jsonExtractFloat('{"a": "hello", "b": [-100, 200.0, 300]}', 'b', 2) = 200.0
select jsonExtractUInt('{"a": "hello", "b": [-100, 200.0, 300]}', 'b', -1) = 300
select jsonExtract('{"a": "hello", "b": [-100, 200.0, 300]}', 'Tuple(String, String, String, Array(Float64))') = ('a', 'hello', 'b', [-100.0, 200.0, 300.0])

The implementation is based on the simdjson library which requires AVX2 instructions.

@vitlibar vitlibar force-pushed the hczhcz-patch-1 branch 4 times, most recently from 1f2fb0c to ca1afbb Compare May 6, 2019 14:29
@vitlibar vitlibar force-pushed the hczhcz-patch-1 branch 8 times, most recently from d34a9c5 to 8490459 Compare May 8, 2019 13:11
@vitlibar vitlibar marked this pull request as ready for review May 8, 2019 13:20
@vitlibar vitlibar changed the title [WIP] Hczhcz patch 1 Hczhcz patch 1 May 8, 2019
@vitlibar vitlibar changed the title Hczhcz patch 1 JSON API based on simdjson May 8, 2019
@vitlibar vitlibar merged commit 49d8f5d into ClickHouse:master May 8, 2019
@vitlibar vitlibar deleted the hczhcz-patch-1 branch May 8, 2019 15:55
@alexey-milovidov
Copy link
Member

It's not production ready (discussion):

Alexey Milovidov, [09.05.19 11:43]
https://github.com/yandex/ClickHouse/pull/5124/files#diff-f0b05095130174676e7ba48481190657R348

@vitlibar @Danlark Какой способ определения возможностей CPU лучше?

Alexey Milovidov, [09.05.19 11:45]
https://github.com/yandex/ClickHouse/blob/master/dbms/src/Core/CpuId.h

Alexey Milovidov, [09.05.19 11:46]
https://github.com/yandex/ClickHouse/blob/master/contrib/libcpuid/include/libcpuid/libcpuid.h#L473

Alexey Milovidov, [09.05.19 11:48]
https://github.com/yandex/ClickHouse/pull/5124/files#diff-4df38c666e503289cb4163b93febffc0R36

Нет комментария, что такое ExtraArg...

Alexey Milovidov, [09.05.19 11:48]
https://github.com/yandex/ClickHouse/pull/5124/files#diff-4df38c666e503289cb4163b93febffc0R40

Нет комментария, что такое Action.

Alexey Milovidov, [09.05.19 11:50]
https://github.com/yandex/ClickHouse/pull/5124/files#diff-4df38c666e503289cb4163b93febffc0R123

Опечатка.

Alexey Milovidov, [09.05.19 11:53]
https://github.com/yandex/ClickHouse/pull/5124/files#diff-4df38c666e503289cb4163b93febffc0R195

Ужасные лишние копирования @vitlibar

Alexey Milovidov, [09.05.19 12:02]
@vitlibar 

Поведение функции jsonLength чем мотивировано?

Alexey Milovidov, [09.05.19 12:03]
@vitlibar Все функции json надо переименовать в JSON, пока не поздно.

Vitaly Baranov, [09.05.19 12:22]
[In reply to Alexey Milovidov]
Это был не мой ПР, я планировал потом сделать еще свой ПР и поправить немного.

Alexey Milovidov, [09.05.19 12:23]
Хорошо. Но лучше хотя бы названия исправить, чтобы  в релиз попали правильные.

Vitaly Baranov, [09.05.19 12:30]
[In reply to Alexey Milovidov]
Там считаются подряд ключи и значения, т.е.
select jsonExtractString('{"a": "hello", "b": "world"}', 1) = 'a'
select jsonExtractString('{"a": "hello", "b": "world"}', 2) = 'hello'
select jsonExtractString('{"a": "hello", "b": "world"}', 3) = 'b'
select jsonExtractString('{"a": "hello", "b": "world"}', 4) = 'world'

Так сделано итерирование в самой библиотеке simdjson и, видимо, это позволило не делать функцию jsonExtractKey().
Хотя да, мне тоже это показалось не слишком логичным, наверное, лучше исправить.

Vitaly Baranov, [09.05.19 12:31]
[In reply to Alexey Milovidov]
почему? вроде у нас довольно много функций начинаются с маленьких букв?

Danila Kutenin, [09.05.19 12:51]
[In reply to Alexey Milovidov]
__builtin_cpu_supports не собирается под другими платформами и не является кроссплатформенным решением https://gcc.godbolt.org/z/iTRS9f

Моя библиотека лучше тем, что она позволяет брать синглтоны и писать просто if (DB::Cpu::CpuFlagsCache::have_AVX2)

Библиотека libcpuid лучше, наверное, тем, что просто больше возможностей предлагает.

Ну и кажется CPU dispatch сделан неправильно в изменении, но это отдельный вопрос

Danila Kutenin, [09.05.19 13:15]
И ещё кажется в libcpuid нет проверки на xgetbv, это может стрелять, но очень редко

https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf 4.1.13

Danila Kutenin, [09.05.19 13:16]
стрелять может, если через OS отключили xmm или ymm

Alexey Milovidov, [09.05.19 13:17]
[In reply to Vitaly Baranov]
Сокращения идут большими буквами, например, URL. Даже когда в начале функции (хотя в остальных случаях функции всегда начинаются с маленькой буквы). Вот для консистентности, придётся назвать с JSON большими буквами.

Alexey Milovidov, [09.05.19 13:18]
@vitlibar Придётся заменить __builtin_cpu_supports на библиотеку @Danlark, а то сейчас билд сломан.

Alexey Milovidov, [09.05.19 13:19]
@Danlark Библиотека CPUID сейчас используется только для определения числа физических ядер. Если ты сделаешь это через свою библиотеку, то можешь её удалить.

Vitaly Baranov, [09.05.19 13:19]
[In reply to Alexey Milovidov]
Какой именно билд сломан?

Alexey Milovidov, [09.05.19 13:20]
[In reply to Vitaly Baranov]
PowerPC.

Vitaly Baranov, [09.05.19 13:20]
[In reply to Alexey Milovidov]
Завтра сделаю ПР.

Danila Kutenin, [09.05.19 13:21]
не сломан вроде

if (NOT HAVE_AVX2)
    message (WARNING "submodule contrib/simdjson requires AVX2 support")
    return()
endif ()

Danila Kutenin, [09.05.19 13:22]
но вообще очень странное изменение, если нет AVX2, функций просто не будет?

Danila Kutenin, [09.05.19 13:22]
Кажется так не принято, но судить не буду

Danila Kutenin, [09.05.19 13:23]
А нет, если компилятор не поддерживает, ок, я ошибся

Vitaly Baranov, [09.05.19 13:27]
[In reply to Danila Kutenin]
Там возможен кейс, когда при сборке AVX2 есть, а в рантайме нет.

Danila Kutenin, [09.05.19 13:30]
Посмотрел ревью, вроде всё правильно, json library собирается отдельным translation unitом

Danila Kutenin, [09.05.19 13:30]
Единственное что, у вас могут тесты флапать, так как в sandbox далеко не всё с AVX2

Vitaly Baranov, [09.05.19 13:42]
[In reply to Alexey Milovidov]
Может, тогда поменять порядок слов? Т.е. вместо JSONExtractString писать extractStringFromJSON ?

Alexey Milovidov, [09.05.19 13:42]
Будет ли слишком отличаться по стилю от visitParam?

Vitaly Baranov, [09.05.19 13:44]
от visitParam оно в любом случае сильно отличается: поведение и набор параметров не такие

Alexey Milovidov, [09.05.19 13:45]
И от dictGet.

Alexey Milovidov, [09.05.19 13:45]
Так что пусть будет JSONExtractString

Vitaly Baranov, [09.05.19 13:46]
ок

Alexey Milovidov, [09.05.19 13:46]
Там хотя бы порядок параметров такой же?
где ищем, что ищем?

Vitaly Baranov, [09.05.19 13:49]
где_ищем, что_ищем, да
только что_ищем может задаваться индексом, а не ключом
и поиск нерекурсивный (ключ ищется только в указанном месте)

Danila Kutenin, [09.05.19 14:03]
[In reply to Danila Kutenin]
Кстати, это реально может быть проблемой. Вы живёте на отдельных машинах?

Alexey Milovidov, [09.05.19 14:39]
[In reply to Danila Kutenin]
Нет, мы используем общее Sandbox облако. Сейчас ограничиваем по SSE 4.2. Есть ограничение на конкретный CPU только для perf тестов.

Alexey Milovidov, [09.05.19 14:41]
[In reply to Vitaly Baranov]
Рекурсивный пользователи спрашивают. Вернее, то, что они хотят - полноценную поддержку функций для работы с JSON. Минимальный вариант будет, если JSONExtractRaw будет работать.

Alexey Milovidov, [09.05.19 19:08]
@vitlibar Нет ни одного теста, показывающего чтение строк с escape последовательностями, в том числе unicode escape последовательностями. В документации не описано, что происходит в этом случае.

@abyss7
Copy link
Contributor

abyss7 commented May 13, 2019

Self-merge, again. I suggest to revert this merge.

@alexey-milovidov
Copy link
Member

It is Ok.

@alexey-milovidov alexey-milovidov self-requested a review May 13, 2019 08:41
@abyss7 abyss7 added the pr-feature Pull request with new product feature label May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants