fix(expr): array_to_string should handle multidimensional array recursively

[xiangjinwu]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Fixes #8680. See #3811 (comment) for rationale.

This is consistent with unnest and cardinality (and array_replace in the future).

Note the following (with ::text[] rather than ::text[][]) is still different from PostgreSQL:

select array_to_string(array[array['one', 'two'], array['three', 'four']]::text[], ',');

This is not related to the array_to_string function. In PostgreSQL, ::text[] still returns a 2d array with 4 elements, while we return a 1d array with 2 elements, first of which is a result of array['one', 'two']::text. This is intentional.

We do not guarantee backward compatibility of experimental multidimensional array.

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
• All checks passed in ./risedev check (or alias, ./risedev c)
• My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

Documentation

• My PR contains user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

• Installation and deployment
• Connector (sources & sinks)
• SQL commands, functions, and operators
• RisingWave cluster configuration changes
• Other (please specify in the release note below)

Release note

[xiangjinwu]

The old .iter().flatten() means to iter 1 layer, then remove nulls.

The new .flatten() means to flatten (unnest) recursively. Nulls are removed via let-else.

The 2 flatten are completely different, one on Iterator<Item=Option<T>> while the other on ListRef

[codecov]

Codecov Report

Merging #10469 (9f2e694) into main (43458ef) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #10469      +/-   ##
==========================================
- Coverage   70.51%   70.50%   -0.01%     
==========================================
  Files        1260     1260              
  Lines      215075   215076       +1     
==========================================
- Hits       151651   151649       -2     
- Misses      63424    63427       +3     

Flag	Coverage Δ
rust	70.50% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/expr/src/expr/expr_array_to_string.rs	2.63% <0.00%> (-0.04%)	⬇️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[st1page]

LGTM

[st1page]

So have we decided to be compatible with PG's behavior for those function with the same name?

[xiangjinwu]

So have we decided to be compatible with PG's behavior for those function with the same name?

Yes. To elaborate:

• It is okay for a query to work in PG but report an error in RW. This is an unsupported feature that may be supported later.
• It is also okay for a query to report an error in PG but work in RW. This is an extension.
• It is better avoided that the same query output different results silently in PG and RW. This is the issue being fixed here.