[SPARK-49968][SQL] The split function produces incorrect results with an empty regex and a limit

[DenineLu]

What changes were proposed in this pull request?

After SPARK-40194, the current behavior of the split function is as follows:

select split('hello', 'h', 1) // result is ["hello"]
select split('hello', '-', 1) // result is ["hello"]
select split('hello', '', 1)  // result is ["h"]

select split('1A2A3A4', 'A', 3) // result is ["1","2","3A4"]
select split('1A2A3A4', '', 3)  // result is ["1","A","2"]

However, according to the function's description, when the limit is greater than zero, the last element of the split result should contain the remaining part of the input string.

Arguments:
      * str - a string expression to split.
      * regex - a string representing a regular expression. The regex string should be a Java regular expression.
      * limit - an integer expression which controls the number of times the regex is applied.
          * limit > 0: The resulting array's length will not be more than `limit`, and the resulting array's last entry will contain all input beyond the last matched regex.
          * limit <= 0: `regex` will be applied as many times as possible, and the resulting array can be of any size. 

So, the split function produces incorrect results with an empty regex and a limit. The correct result should be:

select split('hello', '', 1)    // result is ["hello"]

select split('1A2A3A4', '', 3)  // result is ["1","A","2A3A4"]

Why are the changes needed?

Fix correctness issue.

Does this PR introduce any user-facing change?

Yes.
When the empty regex parameter is provided along with a limit parameter greater than 0, the output of the split function changes.
Before this patch

select split('hello', '', 1)          // result is ["h"]
select split('1A2A3A4', '', 3)  // result is ["1","A","2"]

After this patch

select split('hello', '', 1)          // result is ["hello"]
select split('1A2A3A4', '', 3)  // result is ["1","A","2A3A4"]

How was this patch tested?

Unit tests.

Was this patch authored or co-authored using generative AI tooling?

No.

[TongWei1105]

cc @wangyum @cloud-fan

[wangyum]

cc @vitaliili-db

[uros-db]

thanks for making this change - however, please add collation-related tests as well, see:

test("StringSplit expression with collated strings")

in CollationRegexpExpressionsSuite.scala

[DenineLu]

thanks for making this change - however, please add collation-related tests as well, see:

test("StringSplit expression with collated strings")

in CollationRegexpExpressionsSuite.scala

Thank you for your guidance. The relevant tests have been added.

[uros-db]

Suggested change

	result[i] = UTF8String.fromBytes(input, byteIndex, currCharNumBytes);
	result[charIndex] = UTF8String.fromBytes(input, byteIndex, currCharNumBytes);

[uros-db]

is ArrayIndexOutOfBoundsException possible here?
what if newLimit=0 (i.e. numChars()=0, limit=-1)

[DenineLu]

is ArrayIndexOutOfBoundsException possible here? what if newLimit=0 (i.e. numChars()=0, limit=-1)

no, this code block will only be entered when the following conditions are met.

if (numBytes() != 0 && pattern.numBytes() == 0)

[uros-db]

I would also prefer to see:

SELECT split('', '', -1);
SELECT split('', '', 0);
SELECT split('', '', 1);

here, for more complete testing

[DenineLu]

Thanks, already added.

[uros-db]

Suggested change

	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2", "B", "3", "C"), -1),
	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2", "B", "3", "C"), 0),
	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1A2B3C"), 1),
	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2B3C"), 3),
	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2", "B", "3C"), 5),
	StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2", "B", "3", "C"), 100),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1", "A", "2", "B", "3", "C"), -1),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1", "A", "2", "B", "3", "C"), 0),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1A2B3C"), 1),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1", "A", "2B3C"), 3),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1", "A", "2", "B", "3C"), 5),
	StringSplitTestCase("1A2B3C", "", "UTF8_LCASE", Seq("1", "A", "2", "B", "3", "C"), 100),

sorry, I meant to request using collation (other than UTF8_BINARY) here

[DenineLu]

In the current situation, if UTF8_LCASE is applied to an empty string, the condition here will not be met because the value of pattern after being collated by collationAwareRegex is(?ui), meaning that #37631 does not support truncating the trailing empty string when the pattern is (?ui).

public UTF8String[] split(UTF8String pattern, int limit) {
    // For the empty `pattern` a `split` function ignores trailing empty strings unless original
    // string is empty.
    if (numBytes() != 0 && pattern.numBytes() == 0) {

Therefore, it seems that the result is not what we want when limit <= 0.

select split('1A2B3C', '(?ui)', -1);    // result is ["1", "A", "2", "B", "3", "C", ""]
select split('1A2B3C', '(?ui)', 0);    // result is ["1", "A", "2", "B", "3", "C", ""]

select split('1A2B3C', '(?ui)', 1);    // result is ["1A2B3C"]
select split('1A2B3C', '(?ui)', 3);    // result is ["1", "A", "2B3C"]
select split('1A2B3C', '(?ui)', 6);     // result is ["1", "A", "2", "B", "3", "C"]
select split('1A2B3C', '(?ui)', 100);    // result is ["1", "A", "2", "B", "3", "C"]

When the pattern is "(?ui)", a simple and direct approach can be taken to correct the result.

public UTF8String[] split(UTF8String pattern, int limit) {
    // For the empty `pattern` a `split` function ignores trailing empty strings unless original
    // string is empty.
    if (numBytes() != 0 && (pattern.numBytes() == 0 || lowercaseRegexPrefix.equals(pattern))) {

However, when the pattern is "(?ui)(?ui)" or "(?ui)(?ui)(?ui)", the result still contains a trailing empty string, and I haven't thought of an efficient way to match and resolve it. Should we consider this a reasonable situation?
Additionally, do you think it is necessary to check in the CollationSupport.lowercaseRegex method whether the regex already has a (?ui) prefix?

[uros-db]

left just one more comment, otherwise lgtm (mostly focusing on collation behaviour)

adding @vitaliili-db @cloud-fan to carefully review, extending on #37631

[uros-db]

how about: instead of checking whether the pattern equals to the (?ui) prefix, we modify the collation implementation (prefixing logic) to avoid appending the prefix at all in the case that pattern is an empty string

[DenineLu]

I agree with what you're saying, but should we consider that the user's pattern itself might be (?ui) and is unrelated to prefixing logic?

[uros-db]

that is an interesting observation, although in that case I don't see why the user's pattern can't be any other flag modifier combination, such as: (?m), (?s), (?x), (?a)

taking this into consideration, there is really nothing special about lowercaseRegexPrefix. instead, you should look for a library method that can discern whether a pattern is "functionally" empty, instead of doing a manual check against lowercaseRegexPrefix

[DenineLu]

Thank you for your explanation. It looks like there’s no way to validate this "weird" situation without losing performance. I made changes according to your advice. Thanks again.

[uros-db]

passing on to @vitaliili-db @cloud-fan @MaxGekk for further review