added regexpMatchScalar #15522

Lyra126 · 2024-02-06T16:10:28Z

Summary of the changes / Why this improves CrateDB

Added RegexpMatchScalar

Submitting this pull request as a solution to the following issue: #15117

Created a class, implementing it similar to the existing RegexpMatchOperator.
Instead inherited it from Scalar instead of Operator
Had it support the use of flags.
Updated ScalarFunctionModule.java to register it as a function

Checklist

Added an entry in the latest docs/appendices/release-notes/<x.y.0>.rst for user facing changes
Updated documentation & sql_features table for user facing changes
Touched code is covered by tests
[✓] CLA is signed
[✓] This does not contain breaking changes, or if it does:
- It is released within a major release
- It is recorded in the latest docs/appendices/release-notes/<x.y.0>.rst
- It was marked as deprecated in an earlier release if possible
- You've thought about the consequences and other components are adapted
  (E.g. AdminUI)

crate-jenkins · 2024-02-06T16:10:31Z

Can one of the admins verify this patch?

seut · 2024-02-08T12:07:57Z

@Lyra126
Thank you for this contribution.
Probably I confused you by my comment #15117 (comment) that the scalar is almost equal to the existing operator. But the scalar should return the matched values, so the return type should be a array of strings ->DataTypes.STRING_ARRAY.
See also the example in the original issue request, this example must work.

seut

Could you also add related tests, best by extending from ScalarTestCase please? thanks!

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

seut · 2024-02-14T08:53:05Z

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

+
+    public RegexpMatchScalar(Signature signature, BoundSignature boundSignature) {
+        super(signature, boundSignature);
+    }


Could you please add a compile implementation? By this, any literal pattern will be compiled and flags will be parsed only once and not on each row, resulting in performance improvements.

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

seut · 2024-02-14T10:07:06Z

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

+        int flagSeparatorIndex = pattern.lastIndexOf('/');
+        if (flagSeparatorIndex != -1 && flagSeparatorIndex < pattern.length() - 1) {
+            pattern = pattern.substring(0, flagSeparatorIndex);
+            flagsStr = pattern.substring(flagSeparatorIndex + 1);
+            flags = parseFlags(flagsStr);
+        }


Any optional flag would be the 3rd argument and not part of the pattern, see https://www.postgresql.org/docs/current/functions-matching.html.
To support this, a custom toQuery(Function function, LuceneQueryBuilder.Context context) would be needed to take the 3rd argument into account.

Lyra126 · 2024-02-24T20:28:58Z

Thank you for your continuous review and help with this issue! This is my first time contributing, and I appreciate the help and clarification. Let me know if I'm missing anything else.

seut · 2024-03-11T14:50:58Z

server/src/test/java/io/crate/expression/scalar/regex/regexpMatchScalartest.java

+
+    @Test
+    public void testNormalize() throws Exception {
+        assertNormalize("'foo bar' ~ '([A-Z][^ ]+ ?){2}'", isLiteral(true));


You are testing the existing operator and not your new scalar.
Not exactly sure what you want to test here but this expression will fail in anyway as the regex won't match.
I highly recommend to run your tests yourself ;)

Suggested change

assertNormalize("'foo bar' ~ '([A-Z][^ ]+ ?){2}'", isLiteral(true));

assertNormalize("regexp_match('foo bar', '([A-Z][^ ]+ ?){2}')", isLiteral());

seut · 2024-03-11T14:52:24Z

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

+        compilePattern(patternString, flags);
+    }
+
+    private void compilePattern(String patternString, int flags) throws PatternSyntaxException {


This is not implementing the interface's compile method.. Please check existing implementations to understand how it is used.

seut · 2024-03-11T15:01:49Z

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

+
+        List<String> matches = new ArrayList<>();
+        java.util.regex.Matcher matcher = java.util.regex.Pattern.compile(pattern).matcher(source);
+        while (matcher.find()) {


According to the PG documentation, it should return null if no match is found.
Additionally, the results of the all groups in the current match should be returned, not the first group only but for all global matches (the global flag 'g' must no be supported by this method).

seut · 2024-03-11T15:10:27Z

server/src/main/java/io/crate/expression/scalar/regex/RegexpMatchScalar.java

+
+        // Check if the pattern contains flags
+        String flagsStr = null;
+        int flagSeparatorIndex = pattern.lastIndexOf('/');


I wonder where the flags are added to the pattern by the / separator? I think the existing toQuery implementations won't work as they only support 2 arguments, a reference and a literal, but not 3.

seut · 2024-07-05T12:26:40Z

@Lyra126 Any updates on this, do you need additional help?

Lyra126 · 2024-07-05T16:07:47Z

@Lyra126 Any updates on this, do you need additional help?

Yeah, I’ve realized that I’m a bit lost and may not have the necessary experience to resolve this. I’d like to step back so that someone more familiar with the project can take over. Thank you for checking in though.

seut · 2024-07-08T07:21:33Z

@Lyra126 Ok thank you for trying, I'll close this then.

bhaskar16 · 2024-11-23T04:48:14Z

Hello @seut , can I work on this please?

seut · 2024-11-26T11:52:49Z

@bhaskar16 Sure, go ahead, thanks!
Btw. we stopped assigning tickets as this assignment got stale a lot of times as people did not progress on the development while also not inform us. So just go ahead on working on an issue and create a (Draft) PR asap so we can see your progress.

added regexpMathScalar

c9b62c0

matriv requested a review from seut February 8, 2024 11:47

fixed return value to array of strings

1a3a39a

seut reviewed Feb 14, 2024

View reviewed changes

Added regexpMatchScalartest.java and added suggestions

f114d68

seut reviewed Mar 11, 2024

View reviewed changes

hlcianfagna changed the title ~~added regexpMathScalar~~ added regexpMatchScalar Jul 5, 2024

seut closed this Jul 8, 2024

	assertNormalize("'foo bar' ~ '([A-Z][^ ]+ ?){2}'", isLiteral(true));
	assertNormalize("regexp_match('foo bar', '([A-Z][^ ]+ ?){2}')", isLiteral());

added regexpMatchScalar #15522

added regexpMatchScalar #15522

Uh oh!

Conversation

Lyra126 commented Feb 6, 2024

Summary of the changes / Why this improves CrateDB

Checklist

Uh oh!

crate-jenkins commented Feb 6, 2024

Uh oh!

seut commented Feb 8, 2024

Uh oh!

seut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seut Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seut Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

Lyra126 commented Feb 24, 2024

Uh oh!

seut Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

seut Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

seut Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

seut Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

seut commented Jul 5, 2024

Uh oh!

Lyra126 commented Jul 5, 2024

Uh oh!

seut commented Jul 8, 2024

Uh oh!

bhaskar16 commented Nov 23, 2024

Uh oh!

seut commented Nov 26, 2024

Uh oh!

Uh oh!