fix: regexp_extract returns match in mismatched group

[HolyLow]

The implementation of Re2Extract has a bug, that it might consider a mismatched group as MATCHED empty string "" rather than MISMATCHED std::nullopt.

For example, in the function calling: regexp_extract("rat cat\nbat dog", "ra(.)|blah(.)(.)", 2).
In this case, for group 2 the result must be std::nullopt because no substring would match pattern blah(.).
But the current implementation would mistake the matching of group 1 ra(.) as a empty match case for group 2, and thus return a empty matching, which is wrong.

See detailed description in #12119.

This PR fix this bug in Re2Extract implementation.

Also note that this bug behavior exists in Re2ExtractAll as well, but this PR doesn't modify in Re2ExtractAll because existing UTs of Re2ExtractAll already rely on this behavior.

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	e76c4d3
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67904f0338d3ec0008914b0b

[HolyLow]

@kgpai @mbasmanova Could you kindly help review this PR? Thanks a lot.

Any suggestion is welcome.

[mbasmanova]

Also note that this bug behavior exists in Re2ExtractAll as well, but this PR doesn't modify in Re2ExtractAll because existing UTs of Re2ExtractAll already rely on this behavior.

Sounds like both the implementation is buggy and test expectations are wrong. In this case we need to fix both the implementation and the test.

Would you check Presto Java to see if Velox behavior matches it?

[mbasmanova]

Looks like Presto Java returns NULL:

presto:di> SELECT regexp_extract('rat cat\nbat dog', 'ra(.)|blah(.)(.)', 2);
 _col0
-------
 NULL
(1 row)

@HolyLow Would you create a GitHub issue to describe this problem? Then, reference the issue in the PR description.

[mbasmanova]

Thank you for the fix!

CC: @amitkdutta @kevinwilfong @kagamiori

[facebook-github-bot]

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[HolyLow]

@mbasmanova Bug description issue added.
The fixing for Re2ExractAll will be introduced in next PR as it involves fixing the existing wrong UTs and would take some time.

[mbasmanova]

Thank you, @HolyLow .

[kagamiori]

Hi @HolyLow, there were some build issues last week that caused CI job failures in your PR. They are fixed now. Could you please rebase the PR onto the latest main so that the failed jobs can be re-triggered? Thanks!

[HolyLow]

@kagamiori Rebased to latest main branch as requested.

[facebook-github-bot]

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@kagamiori merged this pull request in 58ff4ac.

[zhouyuan]

It looks like the behavior is not align with Spark-sql:

Spark master: local[*], Application Id: local-1737594817496
spark-sql> SELECT regexp_extract('rat cat\nbat dog', 'ra(.)|blah(.)(.)', 2);

Time taken: 2.226 seconds, Fetched 1 row(s)

For Spark we may need to use a separate impl
Cc: @PHILO-HE