feat: add regexp_match_substring_all function to yaml #469

richtia · 2023-03-09T18:38:55Z

BREAKING CHANGE: group argument added to regexp_match_substring function

Add regexp_match_substring_all function

Resolves #466

richtia · 2023-03-09T18:41:09Z

extensions/functions_string.yaml

ianmcook · 2023-03-09T21:54:43Z

The existing implementation of regexp_match_substring takes the arguments input, pattern, position, occurrence.

The implementation of regexp_match_substring_all proposed here takes the arguments input, pattern, group.

I understand why there is no occurrence argument here. (Because it returns a list of all occurrences.)

But I do not understand:

Why is there no group argument in regexp_match_substring?
Why is there no position argument in regexp_match_substring_all?

richtia · 2023-03-09T22:41:48Z

The existing implementation of regexp_match_substring takes the arguments input, pattern, position, occurrence.

The implementation of regexp_match_substring_all proposed here takes the arguments input, pattern, group.

I understand why there is no occurrence argument here. (Because it returns a list of all occurrences.)

But I do not understand:

Why is there no group argument in regexp_match_substring?

I was wondering the same thing when I submitted this PR. I think this was the case because I was looking at other functions named REGEXP_SUBSTR and those didn't have the group argument (MySQL, Oracle). I can add the group parameter to the function in this PR though.

Why is there no position argument in regexp_match_substring_all?

I wasn't sure whether or not to put the position argument for this function, since I felt like it defeated the purpose of the all part. I don't have a strong opinion either way though. I'm fine with adding it back into this function as well.

ianmcook · 2023-03-09T22:57:06Z

I wasn't sure whether or not to put the position argument for this function, since I felt like it defeated the purpose of the all part. I don't have a strong opinion either way though. I'm fine with adding it back into this function as well.

IIUC, the position argument is still relevant here, even with all. The occurrence argument is the one that is not needed with all.

I think we should aim for these two functions regexp_match_substring and regexp_match_substring_all and their documentation to be identical in all respects except that the former returns one occurrence and the later returns all occurrences.

richtia · 2023-03-09T23:07:06Z

IIUC, the position argument is still relevant here, even with all. The occurrence argument is the one that is not needed with all.

I think we should aim for these two functions regexp_match_substring and regexp_match_substring_all and their documentation to be identical in all respects except that the former returns one occurrence and the later returns all occurrences.

Sounds good, I've updated both.

ianmcook · 2023-03-11T18:46:21Z

extensions/functions_string.yaml

+      on. The `position` argument should be a positive non-zero integer. The regular
+      expression capture group can be specified using the `group` argument. Specifying `0`
+      will match the entire regular expression.  The default group value is `1`, which means the
+      first capture group will be matched. The `group` argument should be a non-negative integer.


@richtia thank you for adding this group argument to the existing regexp_match_substring function. This creates better correspondence between this existing function and the new regexp_match_substring_all function added here. But to be clear, it's not only for the sake of completeness. Multiple real-world functions that extract a single regex match—including R's stringr::str_extract() and Snowflake's with REGEXP_SUBSTR—accept an optional group number argument which allows the user to specify which capture group to return. So this is a practical addition.

Is it considered a breaking change when we add a new argument like this? I forget whether or not Substrait technically allows optional arguments or whether any new argument counts as a breaking change.

I think it would be considered a breaking change. I'm not 100% sure though. @westonpace

Yes, adding a new positional argument is a breaking change (adding a new option is not).

extensions/functions_string.yaml

westonpace

I'm not sure I understand yet but I haven't looked at the regex functions before so it's likely just ignorance on my part.

extensions/functions_string.yaml

westonpace · 2023-03-14T13:05:23Z

extensions/functions_string.yaml

+      Behavior is undefined if the regex fails to compile, the occurrence value is out of range, or
+      the position value is out of range.


Why are these things undefined behaviors and not errors?

FWIW the existing regexp functions in functions_string.yaml use this same kind of boilerplate "Behavior is undefined" language.

Yeah, I think that's just the decision we made during the original PR. But if erroring out is preferred, we can go that route too.

extensions/functions_string.yaml

westonpace

One new question and there is one outstanding (related to undefined behaviors). I've resolved the rest of my comments for clarity.

extensions/functions_string.yaml

ianmcook · 2023-03-21T19:05:38Z

extensions/functions_string.yaml

+      Behavior is undefined if the regex fails to compile, the occurrence value is out of range, or
+      the position value is out of range.


Suggested change

Behavior is undefined if the regex fails to compile, the occurrence value is out of range, or

the position value is out of range.

Behavior is undefined if the regex fails to compile, the occurrence value is out of range,

the position value is out of range, or the group value is out of range.

@richtia could you please also change the "Behavior is undefined" sentence for the regexp_match_substring function to mention the group value?

fix: add group to match_substring and position to match_substring_all docs: update descriptions docs: update description docs: update undefined behavior in match substring

ianmcook · 2023-03-21T20:35:48Z

BREAKING CHANGE: adding a new positional argument

@richtia can you please change the PR description to say:

BREAKING CHANGE: group argument added to regexp_match_substring function

ianmcook · 2023-03-21T20:38:32Z

extensions/functions_string.yaml

+      Behavior is undefined if the regex fails to compile, the occurrence value is out of range,
+      the position value is out of range, or the group value is out of range.


Oops, there is no occurrence value for regexp_match_substring_all

Suggested change

Behavior is undefined if the regex fails to compile, the occurrence value is out of range,

the position value is out of range, or the group value is out of range.

Behavior is undefined if the regex fails to compile, the position value is out of range,

or the group value is out of range.

Oops. Fixed!

westonpace

I'm good. @ianmcook let me know when you're content.

ianmcook · 2023-03-21T23:25:15Z

It looks good to me

westonpace · 2023-03-21T23:33:26Z

+1

Since this is now a breaking extension function change (adding new positional arguments is always a breaking change) it requires two SMC votes.

@cpcloud / @jacques-n ?

ianmcook · 2023-03-22T15:44:08Z

FYI: DuckDB just added a regexp_extract_all function in duckdb/duckdb#6685

cpcloud

LGTM!

richtia requested review from vibhatha and westonpace March 9, 2023 18:40

EpsilonPrime reviewed Mar 9, 2023

View reviewed changes

extensions/functions_string.yaml Show resolved Hide resolved

ianmcook reviewed Mar 11, 2023

View reviewed changes

extensions/functions_string.yaml Outdated Show resolved Hide resolved

ianmcook reviewed Mar 11, 2023

View reviewed changes

extensions/functions_string.yaml Outdated Show resolved Hide resolved

westonpace reviewed Mar 14, 2023

View reviewed changes

richtia force-pushed the regexp_match_substring_all branch 2 times, most recently from f64048e to f5973e8 Compare March 14, 2023 19:32

richtia requested a review from westonpace March 15, 2023 18:30

westonpace requested changes Mar 21, 2023

View reviewed changes

extensions/functions_string.yaml Show resolved Hide resolved

ianmcook reviewed Mar 21, 2023

View reviewed changes

richtia force-pushed the regexp_match_substring_all branch from f5973e8 to 10f9d30 Compare March 21, 2023 19:21

feat: add regexp_match_substring_all function to yaml

6440272

fix: add group to match_substring and position to match_substring_all docs: update descriptions docs: update description docs: update undefined behavior in match substring

richtia force-pushed the regexp_match_substring_all branch from 10f9d30 to 6440272 Compare March 21, 2023 19:21

ianmcook reviewed Mar 21, 2023

View reviewed changes

docs: remove occurrence mention from description

276218c

westonpace approved these changes Mar 21, 2023

View reviewed changes

cpcloud approved these changes Mar 22, 2023

View reviewed changes

richtia merged commit b4d81fb into substrait-io:main Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add regexp_match_substring_all function to yaml #469

feat: add regexp_match_substring_all function to yaml #469

richtia commented Mar 9, 2023 •

edited

Loading

richtia commented Mar 9, 2023

ianmcook commented Mar 9, 2023

richtia commented Mar 9, 2023

ianmcook commented Mar 9, 2023

richtia commented Mar 9, 2023

ianmcook Mar 11, 2023 •

edited

Loading

richtia Mar 13, 2023

westonpace Mar 14, 2023

westonpace left a comment

westonpace Mar 14, 2023

ianmcook Mar 21, 2023

richtia Mar 21, 2023

westonpace left a comment

ianmcook Mar 21, 2023

ianmcook Mar 21, 2023

richtia Mar 21, 2023

ianmcook commented Mar 21, 2023

ianmcook Mar 21, 2023

richtia Mar 21, 2023

westonpace left a comment

ianmcook commented Mar 21, 2023

westonpace commented Mar 21, 2023

ianmcook commented Mar 22, 2023

cpcloud left a comment

		Behavior is undefined if the regex fails to compile, the occurrence value is out of range, or
		the position value is out of range.

		Behavior is undefined if the regex fails to compile, the occurrence value is out of range,
		the position value is out of range, or the group value is out of range.

feat: add regexp_match_substring_all function to yaml #469

feat: add regexp_match_substring_all function to yaml #469

Conversation

richtia commented Mar 9, 2023 • edited Loading

richtia commented Mar 9, 2023

ianmcook commented Mar 9, 2023

richtia commented Mar 9, 2023

ianmcook commented Mar 9, 2023

richtia commented Mar 9, 2023

ianmcook Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace 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

westonpace 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

ianmcook commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace left a comment

Choose a reason for hiding this comment

ianmcook commented Mar 21, 2023

westonpace commented Mar 21, 2023

ianmcook commented Mar 22, 2023

cpcloud left a comment

Choose a reason for hiding this comment

richtia commented Mar 9, 2023 •

edited

Loading

ianmcook Mar 11, 2023 •

edited

Loading