refactor(expr): support `any` type in function signatures #12262

wangrunji0408 · 2023-09-13T06:45:11Z

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

What's changed and what's your intention?

Previously we use DataTypeName for types in function signatures. However:

It doesn't support any type. For functions containing any type, such as array_agg(any) -> anyarray, currently we enumerate all types and generate a function for each specific type. (this is done by expanding the wildcard * in the macro)
It doesn't support particular array type, such as varchar[]. Currently there is only a DataTypeName::List for array type, which is actually equivalent to anyarray. It doesn't have sufficient information, so we have to manually implement type inference for such functions.

To solve these problems, this PR introduces a new type SigDataType to replace DataTypeName in signatures:

pub enum SigDataType {
    Exact(DataType),    // `int`, `varchar[]`...
    Any,                // `any` type
    AnyArray,           // `anyarray` = old List
    AnyStruct,          // = old Struct
}

In #[function] macro:

the previous type list is renamed to anyarray.
a new type any is added. Its corresponding Rust type is ScalarImpl or ScalarRefImpl<'_>.

For example:

#[aggregate("array_agg(any) -> anyarray")]
fn array_agg(state: Option<ListValue>, value: Option<ScalarRefImpl<'_>>) -> ListValue {...}

when the return type is either any or anyarray, the macro can automatically generate a type inference function based on the signature. Here are some examples:
- mode(any) -> any: return_type = args[0]
- array_agg(any) -> anyarray: return_type = List(args[0])
- array_access(anyarray, int32) -> any: return_type = args[0].as_list()
- array_distinct(anyarray) -> anyarray: return_type = args[0]
The rules are following postgres documents:

Each position (either argument or return value) declared as anyelement is allowed to have any specific actual data type, but in any given call they must all be the same actual type. Each position declared as anyarray can have any array data type, but similarly they must all be the same type.
Furthermore, if there are positions declared anyarray and others declared anyelement, the actual array type in the anyarray positions must be an array whose elements are the same type appearing in the anyelement positions.
https://www.postgresql.org/docs/16/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC

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.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

kwannoel · 2023-09-13T09:47:56Z

Can there be more details added to the PR description, on the background of this refactor?

I thought previously we supported any via *, e.g.

#[aggregate("array_agg(*) -> list")]

Or is this different?

wangrunji0408 · 2023-09-13T11:16:21Z

Can there be more details added to the PR description, on the background of this refactor?

I thought previously we supported any via *, e.g.
#[aggregate("array_agg(*) -> list")]
Or is this different?

I'll complete the description tomorrow.

Simply put, we don't support the any type in function signatures. (see Polymorphic Types) Currently we enumerate all types and generate a function for each specific type. (this is done by expanding the wildcard * in the macro)

After this PR, we will have one array_agg function instead of multiple:

- array_agg(int16) -> list
- array_agg(int32) -> list
- array_agg(int64) -> list
 ...
- array_agg(list) -> list
- array_agg(struct) -> list
+ array_agg(any) -> list

And we will get the input as a DatumRef in the function body instead of a specific scalar type:

- fn array_agg<'a>(state: Option<ListValue>, value: Option<impl ScalarRef<'a>>) -> ListValue {
+ fn array_agg<'a>(state: Option<ListValue>, value: Option<ScalarRefImpl<'a>>) -> ListValue {

Signed-off-by: Runji Wang <[email protected]>

xxchan · 2023-09-20T08:26:34Z

After this PR, we will have one array_agg function instead of multiple:

May I ask what's the benefit? 👀

wangrunji0408 · 2023-09-20T08:41:26Z

After this PR, we will have one array_agg function instead of multiple:

May I ask what's the benefit? 👀

Avoiding generating too much code and slowing down compilation? if it counts. 🤡

src/frontend/src/expr/type_inference/func.rs

src/frontend/src/expr/agg_call.rs

src/expr/src/sig/mod.rs

src/expr/macro/src/types.rs

stdrc · 2023-09-25T06:22:29Z

src/expr/src/vector_op/string_to_array.rs

@@ -35,39 +35,33 @@ fn string_to_array_inner<'a>(
 }

 // Use cases shown in `e2e_test/batch/functions/string_to_array.slt.part`


For this, seems docslt is a good choice😁

src/expr/src/agg/general.rs

src/expr/src/sig/mod.rs

src/expr/src/vector_op/array.rs

Signed-off-by: Runji Wang <[email protected]>

stdrc

Rest LGTM!

src/expr/core/src/sig/mod.rs

Signed-off-by: Runji Wang <[email protected]>

codecov · 2023-10-10T16:41:38Z

Codecov Report

Merging #12262 (bf0381a) into main (615f873) will decrease coverage by 0.16%.
Report is 11 commits behind head on main.
The diff coverage is 68.03%.

@@            Coverage Diff             @@
##             main   #12262      +/-   ##
==========================================
- Coverage   69.41%   69.25%   -0.16%     
==========================================
  Files        1473     1471       -2     
  Lines      242568   242507      -61     
==========================================
- Hits       168376   167959     -417     
- Misses      74192    74548     +356

Flag	Coverage Δ
rust	`69.25% <68.03%> (-0.16%)`	⬇️

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

Files	Coverage Δ
src/common/src/array/list_array.rs	`84.14% <100.00%> (+0.06%)`	⬆️
src/expr/impl/src/aggregate/array_agg.rs	`100.00% <100.00%> (ø)`
src/expr/impl/src/aggregate/jsonb_agg.rs	`0.00% <ø> (-34.49%)`	⬇️
src/expr/impl/src/aggregate/mode.rs	`2.04% <100.00%> (ø)`
src/expr/impl/src/aggregate/percentile_disc.rs	`1.69% <100.00%> (ø)`
src/expr/impl/src/aggregate/string_agg.rs	`100.00% <ø> (ø)`
src/expr/impl/src/scalar/array_access.rs	`100.00% <100.00%> (ø)`
src/expr/impl/src/scalar/array_concat.rs	`94.00% <100.00%> (-0.65%)`	⬇️
src/expr/impl/src/table_function/pg_expandarray.rs	`21.42% <100.00%> (-7.15%)`	⬇️
src/expr/impl/src/table_function/unnest.rs	`42.85% <100.00%> (-14.29%)`	⬇️
... and 36 more

... and 47 files with indirect coverage changes

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

xxchan

Can we deprecate * after supporting any? 🤔

xxchan · 2024-07-22T04:36:33Z

src/expr/macro/src/types.rs

+    bytea       Bytea       BytesArray          Box<[u8]>       &[u8]               _
+    jsonb       Jsonb       JsonbArray          JsonbVal        JsonbRef<'_>        _
+    anyarray    List        ListArray           ListValue       ListRef<'_>         _
+    struct      Struct      StructArray         StructValue     StructRef<'_>       _


Why struct is not anystruct?

The name of struct doesn't matter. It is only used internally. In public API, users need to write a full struct schema, like struct<a: int, ...>.

We do write struct in some places 🤪

risingwave/src/expr/impl/src/aggregate/general.rs

Line 103 in dcb565e

#[aggregate("min(struct) -> auto", state = "ref")]

Oh, my bad memory. 🤪

This way, anystruct seems to be a reasonable name.

wangrunji0408 · 2024-07-22T07:39:39Z

Can we deprecate * after supporting any? 🤔

I don't think so. The relationship between * and any is similar to impl T and dyn T in Rust. The performance of using * would be much better than any.

xxchan · 2024-07-22T07:59:58Z

Can we deprecate * after supporting any? 🤔

I don't think so. The relationship between * and any is similar to impl T and dyn T in Rust. The performance of using * would be much better than any.

🤡 Hmmm, if so, shouldn't we prefer * over any instead? It sounds that their expressiveness is the same, but the performance of any is always worse.

wangrunji0408 · 2024-07-22T08:51:48Z

Can we deprecate * after supporting any? 🤔

I don't think so. The relationship between * and any is similar to impl T and dyn T in Rust. The performance of using * would be much better than any.

🤡 Hmmm, if so, shouldn't we prefer * over any instead? It sounds that their expressiveness is the same, but the performance of any is always worse.

Not always, and in some cases, using any is more convenient. For example, in array functions, the element type of ListValue is Scalar(Ref)Impl. It is unnecessary to convert it from/into various T. 🤡

wangrunji0408 requested review from TennyZhuang, xiangjinwu, xxchan and kwannoel September 13, 2023 06:45

github-actions bot added the type/refactor label Sep 13, 2023

wangrunji0408 force-pushed the wrj/varargs branch from 3fe5841 to 398e361 Compare September 13, 2023 13:08

wangrunji0408 force-pushed the wrj/sig branch from 83bd0b0 to 362cc3c Compare September 14, 2023 03:45

Base automatically changed from wrj/varargs to main September 15, 2023 09:21

wangrunji0408 added 13 commits September 19, 2023 14:48

macro: rename "list" to "anyarray"

c07c223

Signed-off-by: Runji Wang <[email protected]>

rename "list" to "anyarray" in function signatures

6b7a6da

Signed-off-by: Runji Wang <[email protected]>

merge function signatures

d33d533

Signed-off-by: Runji Wang <[email protected]>

generate the new signature

f24a7fd

Signed-off-by: Runji Wang <[email protected]>

support any type

ae27c60

Signed-off-by: Runji Wang <[email protected]>

fix all build

8ec6b3a

Signed-off-by: Runji Wang <[email protected]>

fix unit tests

93d3312

Signed-off-by: Runji Wang <[email protected]>

rename function registry

dcea092

Signed-off-by: Runji Wang <[email protected]>

fix unit test

f7cb73c

Signed-off-by: Runji Wang <[email protected]>

auto type inference for any and anyarray

b3f4b47

Signed-off-by: Runji Wang <[email protected]>

fix new functions

27a06f7

Signed-off-by: Runji Wang <[email protected]>

fix build

3dea306

Signed-off-by: Runji Wang <[email protected]>

refine array_sum test

5760603

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 force-pushed the wrj/sig branch from 362cc3c to 5760603 Compare September 19, 2023 06:51

gen: fix "crate" -> "risingwave_expr"

7edd9c0

Signed-off-by: Runji Wang <[email protected]>

stdrc self-requested a review September 19, 2023 06:55

fix clippy

343cb14

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 marked this pull request as ready for review September 19, 2023 08:11

auto type inference for array_agg and mode

31e7c5a

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 force-pushed the wrj/sig branch from e95e7bd to 31e7c5a Compare September 19, 2023 08:12

stdrc mentioned this pull request Sep 20, 2023

feat(agg): introduce an internal agg kind to avoid minput state table after #12349 #12384

Merged

8 tasks

stdrc reviewed Sep 25, 2023

View reviewed changes

wangrunji0408 added 4 commits October 9, 2023 14:40

Merge remote-tracking branch 'origin/main' into wrj/sig

86563e1

Signed-off-by: Runji Wang <[email protected]>

fix panic on _pg_expandarray

c7fea7b

Signed-off-by: Runji Wang <[email protected]>

fix non-unique match snapshot

fca9d45

Signed-off-by: Runji Wang <[email protected]>

resolve comments

2bc5527

Signed-off-by: Runji Wang <[email protected]>

stdrc approved these changes Oct 10, 2023

View reviewed changes

src/expr/core/src/sig/mod.rs Outdated Show resolved Hide resolved

wangrunji0408 added 5 commits October 10, 2023 14:56

fix test

081db38

Signed-off-by: Runji Wang <[email protected]>

fix sqlsmith

940a710

Signed-off-by: Runji Wang <[email protected]>

Merge remote-tracking branch 'origin/main' into wrj/sig

8a63ca3

Signed-off-by: Runji Wang <[email protected]>

fix sqlsmith

d5601eb

Signed-off-by: Runji Wang <[email protected]>

move global functions into FunctionRegistry

69fa3e9

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 enabled auto-merge October 10, 2023 08:28

wangrunji0408 added 2 commits October 10, 2023 23:47

revert type names

3c9dd48

Signed-off-by: Runji Wang <[email protected]>

fix sqlsmith

bf0381a

Signed-off-by: Runji Wang <[email protected]>

wangrunji0408 added this pull request to the merge queue Oct 10, 2023

Merged via the queue into main with commit c952f32 Oct 10, 2023
5 of 7 checks passed

wangrunji0408 deleted the wrj/sig branch October 10, 2023 17:03

ZENOTME mentioned this pull request Jan 11, 2024

feat: support expression to compute iceberg sink partition value #14470

Merged

9 tasks

xxchan reviewed Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(expr): support `any` type in function signatures #12262

refactor(expr): support `any` type in function signatures #12262

wangrunji0408 commented Sep 13, 2023 •

edited

Loading

kwannoel commented Sep 13, 2023

wangrunji0408 commented Sep 13, 2023 •

edited

Loading

xxchan commented Sep 20, 2023

wangrunji0408 commented Sep 20, 2023

stdrc Sep 25, 2023

stdrc left a comment

codecov bot commented Oct 10, 2023 •

edited

Loading

xxchan left a comment

xxchan Jul 22, 2024

wangrunji0408 Jul 22, 2024 •

edited

Loading

xxchan Jul 23, 2024

wangrunji0408 Jul 23, 2024

wangrunji0408 Jul 23, 2024

wangrunji0408 commented Jul 22, 2024

xxchan commented Jul 22, 2024

wangrunji0408 commented Jul 22, 2024

		@@ -35,39 +35,33 @@ fn string_to_array_inner<'a>(
		}

		// Use cases shown in `e2e_test/batch/functions/string_to_array.slt.part`

refactor(expr): support any type in function signatures #12262

refactor(expr): support any type in function signatures #12262

Conversation

wangrunji0408 commented Sep 13, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

kwannoel commented Sep 13, 2023

wangrunji0408 commented Sep 13, 2023 • edited Loading

xxchan commented Sep 20, 2023

wangrunji0408 commented Sep 20, 2023

stdrc Sep 25, 2023

Choose a reason for hiding this comment

stdrc left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

xxchan left a comment

Choose a reason for hiding this comment

xxchan Jul 22, 2024

Choose a reason for hiding this comment

wangrunji0408 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

xxchan Jul 23, 2024

Choose a reason for hiding this comment

wangrunji0408 Jul 23, 2024

Choose a reason for hiding this comment

wangrunji0408 Jul 23, 2024

Choose a reason for hiding this comment

wangrunji0408 commented Jul 22, 2024

xxchan commented Jul 22, 2024

wangrunji0408 commented Jul 22, 2024

refactor(expr): support `any` type in function signatures #12262

refactor(expr): support `any` type in function signatures #12262

wangrunji0408 commented Sep 13, 2023 •

edited

Loading

wangrunji0408 commented Sep 13, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

wangrunji0408 Jul 22, 2024 •

edited

Loading