feat(expr): implement greatest and least function #12838

wenym1 · 2023-10-13T09:45:25Z

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

What's changed and what's your intention?

Complete #12761

The code of cmp unit test is refactored accordingly.

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

Support greatest and least pg function.

GREATEST(value [, ...])

LEAST(value [, ...])

The GREATEST and LEAST functions select the largest or smallest value from a list of any number of expressions. The expressions must all be convertible to a common data type, which will be the type of the result.

NULL values in the argument list are ignored. The result will be NULL only if all the expressions evaluate to NULL. (This is a deviation from the SQL standard. According to the standard, the return value is NULL if any argument is NULL. Some other databases behave this way.)

Refer to https://www.postgresql.org/docs/current/functions-conditional.html#FUNCTIONS-GREATEST-LEAST

TennyZhuang

LGTM

proto/expr.proto

xiangjinwu · 2023-10-16T03:03:15Z

src/frontend/src/binder/expr/function.rs

@@ -672,6 +672,20 @@ impl Binder {
            )
        }

+        fn reduce(r#type: ExprType) -> Handle {


There is variadic function support: #12178

The expressions must all be convertible to a common data type, which will be the type of the result (see Section 10.5 for details).

In the frontend this can be achieved with align_types.

I just checked the function currently using the variadic input. The input type of the function implementations are all f(row: impl Row) like array

#[function("array(...) -> list")] fn array(row: impl Row) -> ListValue { ListValue::new(row.iter().map(|d| d.to_owned_datum()).collect()) }

To implement the functions in this PR, if the input types are also impl Row, we will have to cast the type to the inner type manually, which is verbose and dirty. Is is possible to do things like the following with the proc macro?

fn generic_greatest<T>(input: impl IntoIterator<Item=Option<T>>) -> Option<T> {}

If not I think the current implementation is better.

we will have to cast the type to the inner type manually, which is verbose and dirty

You meant T::try_from? Anyways it would be a backward compatible change from a binary tree to a flat one later. cc @wangrunji0408

Is is possible to do things like the following with the proc macro?

fn generic_greatest<T>(input: impl IntoIterator<Item=Option<T>>) -> Option<T> {}

Yes, it's possible. But it requires some effort to support this in the macro. If this does help, I may add this support in the near future.

we will have to cast the type to the inner type manually, which is verbose and dirty

Can we compare the Datum directly without downcast? I think it should be correct if all inputs have been casted to the common type.

Btw, it seems that implicit cast to the common type is needed in the frontend. Otherwise, the backend function doesn't know how to compare values between string and int.

Can we compare the Datum directly without downcast?

No because there are null first and null last. For the 2 functions here we actually pick neither but ignore nulls.

risingwave/src/common/src/types/mod.rs

Lines 547 to 551 in d2ec83c

// We MUST NOT implement `Ord` for `ScalarImpl` because that will make `Datum` derive an incorrect

// default `Ord`. To get a default-ordered `ScalarImpl`/`ScalarRefImpl`/`Datum`/`DatumRef`, you can

// use `DefaultOrdered<T>`. If non-default order is needed, please refer to `sort_util`.

impl !PartialOrd for ScalarImpl {}

impl !PartialOrd for ScalarRefImpl<'_> {}

I have implemented the variadic version with f(row: impl Row) and T::try_from. The implementation looks much better now. Thanks for the suggestions!

Just fyi I found a case where align_type (pg) is required and a binary expr tree would fail:

dev=> select greatest('12', '2'); greatest ---------- 2 (1 row) dev=> select greatest('12', '2', 0); greatest ---------- 12 (1 row)

The former fallback to varchar/text but the latter infers all 3 to be int. It is not equivalent to greatest(greatest('12', '2'), 0). Not practical, just some PostgreSQL subtleties.

src/expr/impl/src/scalar/cmp.rs

wangrunji0408 · 2023-10-17T05:18:05Z

src/frontend/src/binder/expr/function.rs

@@ -672,6 +672,20 @@ impl Binder {
            )
        }

+        fn reduce(r#type: ExprType) -> Handle {


Btw, it seems that implicit cast to the common type is needed in the frontend. Otherwise, the backend function doesn't know how to compare values between string and int.

xiangjinwu

Rest lgtm.

Feel free to update the unit test expectation on ambiguous signatures to make it pass. It is a false alarm that greatest() requires special return-type inference logic:

greatest() is allowed by the variadic macro, but we have disallowed it in frontend with ensure_arity already.
greatest(..) does have its special return-type inference logic (align_types).

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

src/common/src/monitor/connection.rs

xiangjinwu · 2023-10-17T08:29:21Z

src/expr/impl/src/scalar/cmp.rs

+        .flatten()
+        .map(
+            |scalar| match <<T as Scalar>::ScalarRefType<'_>>::try_from(scalar) {
+                Ok(v) => v.to_owned_scalar(),


It seems we can max on ScalarRef and only to_owned_scalar on the max found at the end.

hzxa21

LGTM.

Need to update the release notes as well.

codecov · 2023-10-17T10:06:34Z

Codecov Report

Merging #12838 (27489fb) into main (1f151cd) will increase coverage by 0.01%.
Report is 7 commits behind head on main.
The diff coverage is 80.81%.

@@            Coverage Diff             @@
##             main   #12838      +/-   ##
==========================================
+ Coverage   69.18%   69.20%   +0.01%     
==========================================
  Files        1490     1490              
  Lines      246047   246110      +63     
==========================================
+ Hits       170235   170318      +83     
+ Misses      75812    75792      -20

Flag	Coverage Δ
rust	`69.20% <80.81%> (+0.01%)`	⬆️

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

Files	Coverage Δ
src/frontend/src/binder/expr/function.rs	`78.44% <100.00%> (+0.05%)`	⬆️
src/frontend/src/expr/pure.rs	`87.69% <ø> (ø)`
src/frontend/src/expr/type_inference/func.rs	`85.56% <0.00%> (-0.22%)`	⬇️
src/expr/impl/src/scalar/cmp.rs	`65.55% <81.43%> (-1.94%)`	⬇️

... and 7 files with indirect coverage changes

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

… function (#12929)

neverchanje · 2023-10-30T14:18:09Z

You missed e2e tests for the streaming mode.

xiangjinwu · 2023-10-31T03:35:46Z

You missed e2e tests for the streaming mode.

These 2 are not aggregate or set returning functions. For pure scalar functions, we do not repeat the test in different configurations.

wenym1 requested review from TennyZhuang, xiangjinwu and st1page October 13, 2023 09:45

github-actions bot added the type/feature label Oct 13, 2023

TennyZhuang approved these changes Oct 13, 2023

View reviewed changes

xiangjinwu reviewed Oct 16, 2023

View reviewed changes

wangrunji0408 reviewed Oct 17, 2023

View reviewed changes

wenym1 force-pushed the yiming/expr-least-greatest branch from c2916a9 to 651d4e3 Compare October 17, 2023 07:59

wenym1 requested review from wangrunji0408 and xiangjinwu October 17, 2023 08:01

xiangjinwu approved these changes Oct 17, 2023

View reviewed changes

feat(expr): implement greatest and least function

eaca70a

wenym1 force-pushed the yiming/expr-least-greatest branch from 651d4e3 to eaca70a Compare October 17, 2023 08:47

call to_owned at the end

cfe55e2

wenym1 enabled auto-merge October 17, 2023 08:52

wenym1 added 2 commits October 17, 2023 17:30

update test result

3bd4d77

use original arity check

27489fb

hzxa21 approved these changes Oct 17, 2023

View reviewed changes

hzxa21 added the user-facing-changes Contains changes that are visible to users label Oct 17, 2023

wenym1 added this pull request to the merge queue Oct 17, 2023

wenym1 added the need-cherry-pick-v1.3 label Oct 17, 2023

Merged via the queue into main with commit 1c2e5aa Oct 17, 2023
38 of 39 checks passed

wenym1 deleted the yiming/expr-least-greatest branch October 17, 2023 10:34

neverchanje mentioned this pull request Oct 17, 2023

Document: feat(expr): implement greatest and least function risingwavelabs/risingwave-docs#1387

Closed

github-actions bot mentioned this pull request Oct 17, 2023

cherrypick feat(expr): implement greatest and least function (#12838) to branch v1.3-rc #12916

Closed

wenym1 mentioned this pull request Oct 18, 2023

refactor: cherry pick #12838 to v1.3-rc to support greatest and least function #12929

Merged

8 tasks

wenym1 added a commit that referenced this pull request Oct 18, 2023

refactor: cherry pick #12838 to v1.3-rc to support greatest and least…

0797ebf

… function (#12929)

neverchanje mentioned this pull request Oct 30, 2023

document greatest and least risingwavelabs/risingwave-docs#1441

Merged

2 tasks

xxchan mentioned this pull request Nov 10, 2023

risingwave 1.4.0 risingwavelabs/homebrew-risingwave#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(expr): implement greatest and least function #12838

feat(expr): implement greatest and least function #12838

wenym1 commented Oct 13, 2023 •

edited

TennyZhuang left a comment

xiangjinwu Oct 16, 2023

wenym1 Oct 16, 2023

xiangjinwu Oct 16, 2023

wangrunji0408 Oct 17, 2023

wangrunji0408 Oct 17, 2023

xiangjinwu Oct 17, 2023

wenym1 Oct 17, 2023

xiangjinwu Oct 17, 2023 •

edited

wangrunji0408 Oct 17, 2023

xiangjinwu left a comment

xiangjinwu Oct 17, 2023

hzxa21 left a comment

codecov bot commented Oct 17, 2023 •

edited

neverchanje commented Oct 30, 2023

xiangjinwu commented Oct 31, 2023

	// We MUST NOT implement `Ord` for `ScalarImpl` because that will make `Datum` derive an incorrect
	// default `Ord`. To get a default-ordered `ScalarImpl`/`ScalarRefImpl`/`Datum`/`DatumRef`, you can
	// use `DefaultOrdered<T>`. If non-default order is needed, please refer to `sort_util`.
	impl !PartialOrd for ScalarImpl {}
	impl !PartialOrd for ScalarRefImpl<'_> {}

feat(expr): implement greatest and least function #12838

feat(expr): implement greatest and least function #12838

Conversation

wenym1 commented Oct 13, 2023 • edited

What's changed and what's your intention?

Checklist

Documentation

Release note

TennyZhuang 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

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

xiangjinwu Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangjinwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2023 • edited

Codecov Report

neverchanje commented Oct 30, 2023

xiangjinwu commented Oct 31, 2023

wenym1 commented Oct 13, 2023 •

edited

xiangjinwu Oct 17, 2023 •

edited

codecov bot commented Oct 17, 2023 •

edited