✨ [SDS-194] Add path_regex to ProximityKeywordsRegex #52

artslidd · 2024-05-23T08:18:27Z

Description

First PR of three that aim to match included keywords against the path of the value that matched.

This PR adds a field called path_regex to the ProximityKeywordsRegex and a function called calculate_keyword_path_pattern that computes the path_pattern using pattern matching.

Some tests verify that the regex generated is correct

fuchsnj · 2024-05-23T20:20:56Z

sds/src/proximity_keywords.rs

@@ -226,11 +241,33 @@ impl Default for Metrics {
    }
 }

+fn compile_keywords_pattern(keyword_patterns: Vec<(Ast, Ast)>) -> (String, String) {
+    let content_pattern = Ast::Alternation(Alternation {


I would expect the Ast::Alternation to be produced by the compile_keywords_to_ast function, since that's part of the Ast, and it doesn't look like that function is used without it anywhere else?

This function would just be calling .to_string(), and could probably just be inlined where it's used

Agreed 👍

Inline compile_keyword_patterns fn

vinckama

I added a few general suggestions in the PR. I think it can be simplified a bit

vinckama · 2024-05-27T11:32:33Z

sds/src/proximity_keywords.rs

+    let content_pattern = Ast::Alternation(Alternation {
+        span: span(),
+        asts: keyword_patterns
+            .iter()
+            .map(|(content_ast, _)| content_ast.clone())
+            .collect(),
+    });

-fn compile_keywords_pattern(keyword_patterns: Vec<Ast>) -> String {
-    Ast::Alternation(Alternation {
+    let path_pattern = Ast::Alternation(Alternation {
        span: span(),
-        asts: keyword_patterns,
-    })
-    .to_string()
+        asts: keyword_patterns
+            .iter()
+            .map(|(_, path_ast)| path_ast.clone())
+            .collect(),
+    });
+
+    Ok(Some((content_pattern, path_pattern)))


It can be simplified with unzip:

let (content_patterns, path_patterns) = keywords... [...] ... .collect::<Result<Vec<_>, _>>()? .iter() .cloned() .unzip(); Ok(Some(( compile_keywords_pattern(content_patterns), compile_keywords_pattern(path_patterns), )))

and where compile_keywords_pattern is (this method can still be inlined if you prefer ):

fn compile_keywords_pattern(keyword_patterns: Vec<Ast>) -> Ast { Ast::Alternation(Alternation { span: span(), asts: keyword_patterns, }) }

Here there is still a cloned, I'm not sure if it's possible to remove it but I think there is less chance to do a mistake by inverting content_patterns and path_pattern.

I'm getting a compilation error saying: "the trait Default is not implemented for regex_syntax::ast::Ast"

I would say that I will come back to this later if that does not bother you (I think it can be left as a todo)

I added an example in this branch

But I checked and it works :), thanks for that 👍

Add unzip to simplify the code

sds/src/proximity_keywords.rs

vinckama · 2024-05-27T12:21:33Z

sds/src/proximity_keywords.rs

+    for chars in chars.windows(3) {
+        let current = &chars[1];
+        let (prev_char, current_char, next_char) = get_chars_type(&chars[0], &chars[1], &chars[2]);
+        match (prev_char, current_char, next_char) {


It seems to me that next_char is not used, it could probably be dropped.

I did not try myself, but maybe you can use a simple for loop on keyword.chars() and keep track on the previous character type (if it exist). This way, the char_list won't need to be created in calculate_keyword_path_pattern, and possibly kw_length >= 2 at the end can be dropped

Improve algorithm and reduce window to 2

artslidd · 2024-05-27T13:06:27Z

@artslidd artslidd deleted a comment from vinckama 17 minutes ago

Sorry for this, I did something wrong in Intellij

Your comment was related to this one, and I fixed them two in this commit: Improve algorithm and reduce window to 2

vinckama

a few more comments

vinckama · 2024-05-28T14:12:54Z

sds/src/proximity_keywords.rs

+
+    push_character(&chars[0]);
+
+    for (i, chars) in chars.windows(2).enumerate() {


Here you can skip the last one and add it ad the end
for chars in chars[..chars.len() - 1].windows(2) {

and then add it later at the end..

The code look like

if chars.is_empty() { return; } push_character(&chars[0]); if chars.len() == 1 { return; } for chars in chars[..chars.len() - 1].windows(2) { .... some logic } push_character(chars.last().unwrap());

I'm not sure it really simplifies the current code, I'm quite happy with the pattern matching that is currently here

It was mostly a suggestion of style to avoid having 3 variables in the match. Both works, I don't have a strong opinion here 👍

vinckama · 2024-05-28T14:13:37Z

sds/src/proximity_keywords.rs

+    let content_pattern = Ast::Alternation(Alternation {
+        span: span(),
+        asts: keyword_patterns
+            .iter()
+            .map(|(content_ast, _)| content_ast.clone())
+            .collect(),
+    });

-fn compile_keywords_pattern(keyword_patterns: Vec<Ast>) -> String {
-    Ast::Alternation(Alternation {
+    let path_pattern = Ast::Alternation(Alternation {
        span: span(),
-        asts: keyword_patterns,
-    })
-    .to_string()
+        asts: keyword_patterns
+            .iter()
+            .map(|(_, path_ast)| path_ast.clone())
+            .collect(),
+    });
+
+    Ok(Some((content_pattern, path_pattern)))


I added an example in this branch

vinckama · 2024-05-28T14:19:37Z

sds/src/proximity_keywords.rs

+            Some(regex_automata::Match::must(0, 14..19))
+        );
+
+        assert_eq!(path_regex.is_match("awsAccess"), false);


should it be a match as aws access in pascalCase is awsAccess?

Actually no because I'm expecting to have the path being transformed in aws.access so it should not match the camel case: awsAccess

artslidd force-pushed the artslidd/sds-194-generate-path-regex branch 2 times, most recently from 01b42f1 to f8d5036 Compare May 23, 2024 09:39

artslidd changed the title ~~Artslidd/sds 194 generate path regex~~ ✨ [SDS-194] Add path_regex to ProximityKeywordsRegex May 23, 2024

artslidd added 3 commits May 23, 2024 11:42

💬 Rename function and regex to content_regex

dbaac3e

✨ Add path_regex to ProximityKeywordsRegex

0ed8562

✅ Fix tests and improve code

29ac559

artslidd force-pushed the artslidd/sds-194-generate-path-regex branch from f8d5036 to 29ac559 Compare May 23, 2024 09:42

artslidd added 3 commits May 23, 2024 11:53

Fix clippy warnings

45e1a2f

Improve code

c69e59b

Add test case covering word boundaries

7269c8b

artslidd marked this pull request as ready for review May 23, 2024 13:57

fuchsnj reviewed May 23, 2024

View reviewed changes

Inline compile_keyword_patterns fn

94bdea5

artslidd force-pushed the artslidd/sds-194-generate-path-regex branch from de96e32 to 94bdea5 Compare May 24, 2024 08:08

artslidd requested a review from fuchsnj May 24, 2024 11:16

artslidd added 2 commits May 27, 2024 11:38

Improve algorithm and make it reusable

f706b64

Fix clippy warnings

8221a8c

vinckama reviewed May 27, 2024

View reviewed changes

Simplify should_push_word_boundary

df46a32

DataDog deleted a comment from vinckama May 27, 2024

Improve algorithm and reduce window to 2

44c6974

artslidd requested a review from vinckama May 28, 2024 06:27

vinckama reviewed May 28, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

vinckama reviewed May 28, 2024

View reviewed changes

Add unzip to simplify the code

874dd3b

vinckama approved these changes May 28, 2024

View reviewed changes

Merge branch 'main' into artslidd/sds-194-generate-path-regex

b67635a

artslidd merged commit c425920 into main May 28, 2024
4 checks passed

artslidd deleted the artslidd/sds-194-generate-path-regex branch May 28, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [SDS-194] Add path_regex to ProximityKeywordsRegex #52

✨ [SDS-194] Add path_regex to ProximityKeywordsRegex #52

artslidd commented May 23, 2024 •

edited

Loading

fuchsnj May 23, 2024

artslidd May 24, 2024

artslidd May 24, 2024

vinckama left a comment •

edited

Loading

vinckama May 27, 2024

artslidd May 27, 2024

artslidd May 27, 2024

vinckama May 28, 2024

artslidd May 28, 2024

artslidd May 28, 2024

vinckama May 27, 2024

artslidd May 27, 2024

artslidd commented May 27, 2024

vinckama left a comment

vinckama May 28, 2024

artslidd May 28, 2024

vinckama May 28, 2024

vinckama May 28, 2024

This comment was marked as duplicate.

vinckama May 28, 2024

artslidd May 28, 2024


		push_character(&chars[0]);

		for (i, chars) in chars.windows(2).enumerate() {

✨ [SDS-194] Add path_regex to ProximityKeywordsRegex #52

✨ [SDS-194] Add path_regex to ProximityKeywordsRegex #52

Conversation

artslidd commented May 23, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinckama left a comment • 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

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

artslidd commented May 27, 2024

vinckama left a 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

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artslidd commented May 23, 2024 •

edited

Loading

vinckama left a comment •

edited

Loading