From be8e0487994d70c3c6350cb675475b12dffda8e5 Mon Sep 17 00:00:00 2001 From: johanngan Date: Tue, 6 Jun 2023 16:28:44 -0500 Subject: [PATCH] Revert invalid RegExpTreeDictionary optimization This reverts the following commits: - e77dd810369ad5fcf957393e4fc71a8a6220b04e - e8527e720b2ab12b3327f1e3886aace402a292c6 Additionally, functional tests are added. When scanning complex regexp nodes sequentially with RE2, the old code has an optimization to break out of the loop early upon finding a leaf node that matches. This is an invalid optimization because there's no guarantee that it's actually a VALID match, because its parents might NOT have matched. Semantically, a user would expect this match to be discarded and for the search to continue. Instead, since we skipped matching after the first false positive, subsequent nodes that would have matched are missing from the output value. This affects both dictGet and dictGetAll. It's difficult to distinguish a true positive from a false positive while looping through complex_regexp_nodes because we would have to scan all the parents of a matching node to confirm a true positive. Trying to do this might actually end up being slower than just scanning every complex regexp node, because complex_regexp_nodes is only a subset of all the tree nodes; we may end up duplicating work with scanning that Vectorscan has already done, depending on whether the parent nodes are "simple" or "complex". So instead of trying to fix this optimization, just remove it entirely. --- src/Dictionaries/RegExpTreeDictionary.cpp | 14 ---- ...04_regexp_dictionary_yaml_source.reference | 16 +++++ .../02504_regexp_dictionary_yaml_source.sh | 64 +++++++++++++++++++ 3 files changed, 80 insertions(+), 14 deletions(-) diff --git a/src/Dictionaries/RegExpTreeDictionary.cpp b/src/Dictionaries/RegExpTreeDictionary.cpp index 8d0af9b0abf..3852cca6928 100644 --- a/src/Dictionaries/RegExpTreeDictionary.cpp +++ b/src/Dictionaries/RegExpTreeDictionary.cpp @@ -129,17 +129,6 @@ struct RegExpTreeDictionary::RegexTreeNode return searcher.Match(haystack, 0, size, re2_st::RE2::Anchor::UNANCHORED, nullptr, 0); } - /// check if this node can cover all the attributes from the query. - bool containsAll(const std::unordered_map & matching_attributes) const - { - for (const auto & [key, value] : matching_attributes) - { - if (!attributes.contains(key)) - return false; - } - return true; - } - struct AttributeValue { Field field; @@ -691,9 +680,6 @@ std::unordered_map RegExpTreeDictionary::match( if (node_ptr->match(reinterpret_cast(keys_data.data()) + offset, length)) { match_result.insertNodeID(node_ptr->id); - /// When this node is leaf and contains all the required attributes, it means a match. - if (node_ptr->containsAll(attributes) && node_ptr->children.empty()) - break; } } diff --git a/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.reference b/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.reference index 437012dd516..79871e3716c 100644 --- a/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.reference +++ b/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.reference @@ -11,3 +11,19 @@ (['ClickHouse Documentation','ClickHouse'],[0,1],['/en'],['ClickHouse']) (['Documentation','GitHub'],[2,3],[NULL],[]) (['Documentation','GitHub'],[2,3],[NULL],[]) +ClickHouse +['ClickHouse'] +ClickHouse Documentation +['ClickHouse Documentation','ClickHouse','Documentation'] +GitHub Documentation +['GitHub Documentation','GitHub'] +Documentation +['Documentation'] +ClickHouse +['ClickHouse'] +ClickHouse Documentation +['ClickHouse Documentation','ClickHouse','Documentation'] +GitHub Documentation +['GitHub Documentation','GitHub'] +Documentation +['Documentation'] diff --git a/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh b/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh index ac0793460a9..5e8985406ae 100755 --- a/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh +++ b/tests/queries/0_stateless/02504_regexp_dictionary_yaml_source.sh @@ -175,6 +175,70 @@ select dictGetAll('regexp_dict3', ('tag', 'topological_index', 'captured', 'pare select dictGetAll('regexp_dict3', ('tag', 'topological_index', 'captured', 'parent'), 'github.com/clickhouse/tree/master/docs', 2); " +# Test that things work the same for "simple" regexps that go through Hyperscan and "complex" regexps that go through RE2. +# An easy way to force the use of RE2 is to disable Hyperscan. +# This tree is constructed purposely so that text might (falsely) match leaf nodes without matching their corresponding parent nodes +cat > "$yaml" <