Merge #40878: Add regexp tree dictionary by hanfei1991 · Pull Request #43858 · ClickHouse/ClickHouse

hanfei1991 · 2022-12-01T10:54:57Z

Changelog category (leave one):

New Feature

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

to merge #40878 , supporting regexp dictionary.

The YAML source has a new format. For every yaml node, the format will be like:

key_name (defined by dictioanary structure) : regular expression
attribute_name (defined by dictionary structure) : value (might be a string containing back-reference)
... (multiple attribute names)
name which is not key or attributes: another yaml node (for recursive match)

This format is to keep compatible with: https://github.com/ua-parser/uap-core and https://github.com/matomo-org/device-detector/tree/master/regexes

The match order is :

son is higher than parents,
smaller id is highter than larger id if they are in the same layer.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

hanfei1991 · 2022-12-23T12:41:13Z

Next goal is:

Support popular ua-parser lib his format is to keep compatible with: https://github.com/ua-parser/uap-core and https://github.com/matomo-org/device-detector/tree/master/regexes , the problem with them right now is:
- the regular expressions in the uap-core include finite boundries like "{0,200}" which cause huge pressure on vectorscan. we should refine these re before we test this lib
- the regular expressions in device-detector contain zero-length assertions which is not supported by vectorscan. I will see if it's possible to avoid this syntax.
Support more types parsing in yaml file and handle exceptions for parse error.

The other parts are ready to review. PTAL @vdimir

vdimir

Sorry for the delay.
The code almost looks good, just some comments. Also, It's worth adding some docs (in this or followup PR).

tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh

src/Common/ErrorCodes.cpp

src/Dictionaries/YAMLRegExpTreeDictionarySource.cpp

vdimir · 2022-12-28T10:18:48Z

src/Dictionaries/RegExpTreeDictionary.cpp

+    for (const auto & item : pieces)
+    {
+        if (item.ref_num >= 0)
+            result += matches[item.ref_num].ToString();


We already checked ref_num in createStringPieces, but let's also add check here to be sure that item.ref_num < 10 and we don't overflow matches array

vdimir · 2022-12-28T10:24:47Z

src/Dictionaries/RegExpTreeDictionary.cpp

+    const StorageID & id_, const DictionaryStructure & structure_, DictionarySourcePtr source_ptr_, Configuration configuration_)
+    : IDictionary(id_), structure(structure_), source_ptr(source_ptr_), configuration(configuration_), logger(&Poco::Logger::get("RegExpTreeDictionary"))
+{
+    if (auto * ch_source = typeid_cast<ClickHouseDictionarySource *>(source_ptr.get()))


How sample_block will work if source is other than Clickhouse ? Is it safe to change ClickHouseDictionarySource::sample_block after ClickHouseDictionarySource was created?

if source is not ClickHouse, the schema will mismatch and raise a Exception.

This is only for test. We assume that the user only uses YAML source by default.

vdimir · 2022-12-28T10:27:02Z

tests/queries/0_stateless/02504_regexp_dictionary_table_source.sql

+    comment String default 'nothing'
+)
+PRIMARY KEY(regexp)
+SOURCE(CLICKHOUSE(QUERY concat('select * from ', currentDatabase() , '.regexp_dictionary_source_table')))


Why it's not just SOURCE(CLICKHOUSE(TABLE 'regexp_dictionary_source_table'))?

because the schema will mismatch if we refer a TABLE. Though if we refer a QUERY, the schema will be decided by sample_block. That's trikcy but I think it's ok for test.

vdimir · 2022-12-28T10:28:16Z

src/Dictionaries/RegExpTreeDictionary.cpp

+    if (auto * ch_source = typeid_cast<ClickHouseDictionarySource *>(source_ptr.get()))
+    {
+        Block sample_block;
+        /// id, parent_id, regex, keys, values


How exactly existing sample_block differ? In tests we have the same

CREATE TABLE regexp_dictionary_source_table ( id UInt64, parent_id UInt64, regexp String, keys Array(String), values Array(String),

Yeah but the schema of Dictionary is (regexp, attribute_1, attribute_2 ...) . ClickHouse assumes dictionary and ClickHouse source have same schema

vdimir · 2022-12-28T10:32:14Z

src/Dictionaries/YAMLRegExpTreeDictionarySource.cpp

+*   - regexp: "MSIE (\\d+)"
+*     attr1: "Vecna"
+*     attr2: 22.8
+*     match: # nested match for subpattern


in tests it's named versions, so what is the correct way to define subpath in this tree?

indeed, the subpath attrubute name can be arbitrary.

Co-authored-by: Vladimir C <[email protected]>

hanfei1991 · 2022-12-30T16:36:34Z

hanfei1991 · 2023-01-06T15:02:12Z

@vdimir PTAL. I tested more data types and added restrictions for source types. Now it's safer than before. I also added some simple docs with simple examples. Not thouroughly but enough for a good begin :)

vdimir

LGTM

docs/en/sql-reference/dictionaries/external-dictionaries/regexp-tree.md

…p-tree.md Co-authored-by: Vladimir C <[email protected]>

tavplubix · 2023-04-04T16:25:52Z

src/Dictionaries/RegExpTreeDictionary.cpp

+    if (topology_order.size() != regex_nodes.size())
+        throw Exception(ErrorCodes::INCORRECT_DICTIONARY_DEFINITION, "Invalid Regex tree");


This exception message is not really informative for a user, could you please update it (or replace with LOGICAL ERROR if it's not for a user)? (what is invalid? how to fix it?)

actually this belongs to LOGICAL_ERROR. fixing in #48632
Thank you!

tavplubix · 2023-04-04T16:26:31Z

src/Dictionaries/RegExpTreeDictionary.cpp

+        if (visited.contains(child_idx))
+            throw Exception(ErrorCodes::INCORRECT_DICTIONARY_DEFINITION, "Invalid Regex tree");


nooblose and others added 6 commits August 24, 2022 14:01

Add YAML regular expressions tree dictionary source

540fa7f

Add RegExpTree dictionary

9b2326c

dummy commit

8245c5c

Merge branch 'master' into regexp-tree-dictionary

27ec6bc

fix compile

41f726c

fix macro flag

e28c98d

hanfei1991 assigned vdimir Dec 1, 2022

robot-ch-test-poll1 added the pr-feature Pull request with new product feature label Dec 1, 2022

hanfei1991 added 3 commits December 1, 2022 13:29

fix check-style

ae91d0c

fix style check

2330454

remove integration tests to check CI

a52886f

hanfei1991 changed the title ~~Merge #40878~~ [WIP] Merge #40878 Dec 6, 2022

hanfei1991 added 5 commits December 14, 2022 16:28

reimplement

2272d71

Merge branch 'master' into regexp-tree-dictionary

d3f8bb3

support regex tree dictionary

efa963f

fix build

2bb952a

fix build

4859197

hanfei1991 changed the title ~~[WIP] Merge #40878~~ Merge #40878 Dec 22, 2022

hanfei1991 added 2 commits December 23, 2022 12:01

use vector scan in test

b3ca552

add test for yaml source

0cd1992

hanfei1991 mentioned this pull request Dec 23, 2022

regexp_tree dictionary type #21694

Closed

vdimir changed the title ~~Merge #40878~~ Merge #40878: Add regexp tree dictionary Dec 28, 2022

vdimir reviewed Dec 28, 2022

View reviewed changes

hanfei1991 and others added 6 commits December 29, 2022 18:33

Update tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh

b4798cf

Co-authored-by: Vladimir C <[email protected]>

Update tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh

1fb621c

Co-authored-by: Vladimir C <[email protected]>

address comments

50905e2

add check sanitizer

fa1baef

try to fix flaky tests

83c6517

Merge branch 'master' into regexp-tree-dictionary

c25207f

hanfei1991 added 9 commits December 31, 2022 20:36

fix style check

97cdfdc

some improvement

00e717d

fix build

65ef7b4

refine test

7f70568

write docs and optimize regex compile

f2a9eea

fix check style

ae5ee81

fix build

7440843

fix build

cac7f65

fix build

a4427a0

alexey-milovidov mentioned this pull request Jan 7, 2023

Supposedly fix the "Download script failed" error #45013

Merged

vdimir approved these changes Jan 10, 2023

View reviewed changes

docs/en/sql-reference/dictionaries/external-dictionaries/regexp-tree.md Outdated Show resolved Hide resolved

hanfei1991 and others added 3 commits January 10, 2023 14:41

Update docs/en/sql-reference/dictionaries/external-dictionaries/regex…

5f8296b

…p-tree.md Co-authored-by: Vladimir C <[email protected]>

Merge branch 'master' into regexp-tree-dictionary

6ed4570

Merge branch 'master' into regexp-tree-dictionary

b911c3f

hanfei1991 merged commit 91226ab into ClickHouse:master Jan 10, 2023

hanfei1991 deleted the regexp-tree-dictionary branch January 10, 2023 15:41

DanRoscigno mentioned this pull request Jan 13, 2023

Release readiness for 23.1 ClickHouse/clickhouse-docs#671

Closed

15 tasks

tavplubix reviewed Apr 4, 2023

View reviewed changes

tavplubix mentioned this pull request Apr 4, 2023

Find big allocations without memory limits checks #48401

Merged

		if (topology_order.size() != regex_nodes.size())
		throw Exception(ErrorCodes::INCORRECT_DICTIONARY_DEFINITION, "Invalid Regex tree");

		if (visited.contains(child_idx))
		throw Exception(ErrorCodes::INCORRECT_DICTIONARY_DEFINITION, "Invalid Regex tree");

Conversation

hanfei1991 commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

hanfei1991 commented Dec 23, 2022

Uh oh!

vdimir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanfei1991 commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jobs needed to be done in this pr:

jobs after this pr:

Uh oh!

hanfei1991 commented Jan 6, 2023

Uh oh!

vdimir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hanfei1991 commented Dec 1, 2022 •

edited

Loading

hanfei1991 commented Dec 30, 2022 •

edited

Loading