feat(binder): Support bind args in Ordered-Set Agg #10193

Honeta · 2023-06-06T09:53:17Z

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

What's changed and what's your intention?

as title

Checklist For Contributors

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).
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

My PR DOES NOT contain 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

src/expr/src/agg/def.rs

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

Co-authored-by: stonepage <40830455+st1page@users.noreply.github.com>

codecov · 2023-06-06T11:22:13Z

Codecov Report

Merging #10193 (9734d57) into main (64ec79f) will increase coverage by 0.01%.
The diff coverage is 84.11%.

@@            Coverage Diff             @@
##             main   #10193      +/-   ##
==========================================
+ Coverage   70.73%   70.74%   +0.01%     
==========================================
  Files        1237     1237              
  Lines      211683   211831     +148     
==========================================
+ Hits       149726   149852     +126     
- Misses      61957    61979      +22

Flag	Coverage Δ
rust	`70.74% <84.11%> (+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_literal.rs	`97.36% <0.00%> (ø)`
src/frontend/src/expr/expr_rewriter.rs	`51.96% <0.00%> (ø)`
src/frontend/src/expr/mod.rs	`79.49% <0.00%> (-0.13%)`	⬇️
src/expr/src/agg/def.rs	`51.21% <43.75%> (-0.67%)`	⬇️
...rc/frontend/src/optimizer/plan_node/generic/agg.rs	`82.28% <63.63%> (-0.37%)`	⬇️
src/frontend/src/binder/expr/function.rs	`87.30% <84.84%> (-0.22%)`	⬇️
src/batch/src/executor/hash_agg.rs	`89.87% <100.00%> (+0.08%)`	⬆️
src/batch/src/executor/sort_agg.rs	`93.91% <100.00%> (+0.04%)`	⬆️
src/expr/src/agg/array_agg.rs	`97.61% <100.00%> (+0.05%)`	⬆️
src/expr/src/agg/general.rs	`89.47% <100.00%> (+0.05%)`	⬆️
... and 14 more

... and 4 files with indirect coverage changes

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

st1page

please add some planner test, especially for those error branches

st1page · 2023-06-08T08:19:16Z

why fe do not return the error to pgwire 🤔

dev=>     create table t (x int, y int);
CREATE_TABLE
dev=>     select percentile_cont(0, 0) within group (order by y) from t;

 percentile_cont 
-----------------
(0 rows)

2023-06-08T08:10:26.604994Z ERROR risingwave_batch::executor: Failed to build executor: Expr error: Unsupported function: percentile_cont(int32) -> int32, plan node is: 
 Ok(SortAgg(SortAggNode { group_key: [], agg_calls: [AggCall { r#type: PercentileCont, args: [InputRef { index: 0, r#type: Some(DataType { type_name: Int32, precision: 0, scale: 0, is_nullable: true, interval_type: Unspecified, field_type: [], field_names: [] }) }], return_type: Some(DataType { type_name: Int32, precision: 0, scale: 0, is_nullable: true, interval_type: Unspecified, field_type: [], field_names: [] }), distinct: false, order_by: [ColumnOrder { column_index: 0, order_type: Some(OrderType { direction: Ascending, nulls_are: Largest }) }], filter: None, direct_args: [Constant { datum: Some(Datum { body: [1, 0, 0, 0, 0] }), r#type: Some(DataType { type_name: Int32, precision: 0, scale: 0, is_nullable: true, interval_type: Unspecified, field_type: [], field_names: [] }) }, Constant { datum: Some(Datum { body: [1, 0, 0, 0, 0] }), r#type: Some(DataType { type_name: Int32, precision: 0, scale: 0, is_nullable: true, interval_type: Unspecified, field_type: [], field_names: [] }) }] }] }))
2023-06-08T08:10:26.607839Z ERROR risingwave_frontend::scheduler::distributed::stage: Stage QueryId { id: "39bc9d8e-bb04-429f-b518-244ec953e3b8" }-0 failed to schedule tasks, error: Internal(internal error: Failed to build executor: Expr error: Unsupported function: percentile_cont(int32) -> int32

Caused by:
    internal error: Failed to build executor: Expr error: Unsupported function: percentile_cont(int32) -> int32

Stack backtrace:
   0: anyhow::error::<impl core::convert::From<E> for anyhow::Error>::from
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.71/src/error.rs:547:25
   1: <T as core::convert::Into<U>>::into
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/core/src/convert/mod.rs:717:9
   2: <risingwave_frontend::scheduler::error::SchedulerError as core::convert::From<risingwave_common::error::RwError>>::from
             at ./src/frontend/src/scheduler/error.rs:69:24
   3: <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/core/src/result.rs:1961:27
   4: risingwave_frontend::scheduler::distributed::stage::StageRunner::schedule_tasks_for_root::{{closure}}
             at ./src/frontend/src/scheduler/distributed/stage.rs:566:24
   5: risingwave_frontend::scheduler::distributed::stage::StageRunner::schedule_tasks_for_all::{{closure}}
             at ./src/frontend/src/scheduler/distributed/stage.rs:626:55
   6: risingwave_frontend::scheduler::distributed::stage::StageRunner::run::{{closure}}
             at ./src/frontend/src/scheduler/distributed/stage.rs:301:66
   7: risingwave_frontend::scheduler::distributed::stage::StageExecution::start::{{closure}}::{{closure}}
             at ./src/frontend/src/scheduler/distributed/stage.rs:220:57
   8: <core::pin::Pin<P> as core::future::future::Future>::poll
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/core/src/future/future.rs:125:9
   9: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tracing-0.1.37/src/instrument.rs:272:9
  10: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/core.rs:223:17
  11: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/loom/std/unsafe_cell.rs:14:9
  12: tokio::runtime::task::core::Core<T,S>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/core.rs:212:13
  13: tokio::runtime::task::harness::poll_future::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:476:19
  14: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/core/src/panic/unwind_safe.rs:271:9
  15: std::panicking::try::do_call
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panicking.rs:500:40
  16: __rust_try
  17: std::panicking::try
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panicking.rs:464:19
  18: std::panic::catch_unwind
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panic.rs:142:14
  19: tokio::runtime::task::harness::poll_future
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:464:18
  20: tokio::runtime::task::harness::Harness<T,S>::poll_inner
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:198:27
  21: tokio::runtime::task::harness::Harness<T,S>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:152:15
  22: tokio::runtime::task::raw::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/raw.rs:255:5
  23: tokio::runtime::task::raw::RawTask::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/raw.rs:200:18
  24: tokio::runtime::task::LocalNotified<S>::run
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/mod.rs:394:9
  25: tokio::runtime::scheduler::multi_thread::worker::Context::run_task::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:487:21
  26: tokio::runtime::coop::with_budget
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/coop.rs:107:5
  27: tokio::runtime::coop::budget
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/coop.rs:73:5
  28: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:463:9
  29: tokio::runtime::scheduler::multi_thread::worker::Context::run
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:426:24
  30: tokio::runtime::scheduler::multi_thread::worker::run::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:406:17
  31: tokio::macros::scoped_tls::ScopedKey<T>::set
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/macros/scoped_tls.rs:61:9
  32: tokio::runtime::scheduler::multi_thread::worker::run
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:403:5
  33: tokio::runtime::scheduler::multi_thread::worker::Launch::launch::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/scheduler/multi_thread/worker.rs:365:45
  34: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/blocking/task.rs:42:21
  35: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tracing-0.1.37/src/instrument.rs:272:9
  36: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/core.rs:223:17
  37: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/loom/std/unsafe_cell.rs:14:9
  38: tokio::runtime::task::core::Core<T,S>::poll
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/core.rs:212:13
  39: tokio::runtime::task::harness::poll_future::{{closure}}
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:476:19
  40: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/core/src/panic/unwind_safe.rs:271:9
  41: std::panicking::try::do_call
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panicking.rs:500:40
  42: __rust_try
  43: std::panicking::try
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panicking.rs:464:19
  44: std::panic::catch_unwind
             at /rustc/f0411ffcebcd7f75ac02ed45feb53ffd07b75398/library/std/src/panic.rs:142:14
  45: tokio::runtime::task::harness::poll_future
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.0/src/runtime/task/harness.rs:464:18
  46: tokio::runtime::task::harness::Harness<T,S>::poll_inner
             at /home/sp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-

stdrc

LGTM

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

bind ordered set agg

f6f6148

Honeta requested review from stdrc and st1page June 6, 2023 09:53

Merge branch 'main' into xinjing/bind_ordered_set_agg

9c6c676

github-actions bot added the type/feature label Jun 6, 2023

add to do

a43ee01

st1page reviewed Jun 6, 2023

View reviewed changes

src/expr/src/agg/def.rs Outdated Show resolved Hide resolved

src/frontend/src/binder/expr/function.rs Show resolved Hide resolved

src/frontend/src/binder/expr/function.rs Show resolved Hide resolved

src/frontend/src/binder/expr/function.rs Show resolved Hide resolved

Honeta and others added 8 commits June 6, 2023 18:19

Update src/expr/src/agg/def.rs

2da4935

Co-authored-by: stonepage <40830455+st1page@users.noreply.github.com>

Merge branch 'main' into xinjing/bind_ordered_set_agg

79d87e0

fix check

22a86bc

ensure within_group exists if expected

e87d056

fix check

a7c6739

Merge branch 'main' into xinjing/bind_ordered_set_agg

47ec46e

impl to_protobuf

c4e0f60

order_by clause outside of within group is disallowed

3e35e99

Honeta added 3 commits June 7, 2023 11:16

Merge branch 'main' into xinjing/bind_ordered_set_agg

e2dc370

fix return_type

be9d2c0

Merge branch 'main' into xinjing/bind_ordered_set_agg

476ab0f

st1page reviewed Jun 8, 2023

View reviewed changes

Honeta added 3 commits June 8, 2023 07:23

fix direct arg type

9f233c0

add planner test

e45215f

Merge branch 'main' into xinjing/bind_ordered_set_agg

a4a5d7c

Honeta requested a review from st1page June 8, 2023 07:27

fix check

c797340

Honeta added 2 commits June 8, 2023 08:21

add another planner test

c7ebcea

Merge branch 'main' into xinjing/bind_ordered_set_agg

172540c

stdrc approved these changes Jun 8, 2023

View reviewed changes

src/frontend/src/binder/expr/function.rs Outdated Show resolved Hide resolved

improve code

53af7e8

Merge branch 'main' into xinjing/bind_ordered_set_agg

9734d57

Honeta enabled auto-merge June 8, 2023 11:22

Honeta added this pull request to the merge queue Jun 8, 2023

Merged via the queue into main with commit 035aae7 Jun 8, 2023

Honeta deleted the xinjing/bind_ordered_set_agg branch June 8, 2023 11:54

Honeta mentioned this pull request Jun 9, 2023

Feat(SQL): Ordered-Set Aggregate Functions #10038

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(binder): Support bind args in Ordered-Set Agg #10193

feat(binder): Support bind args in Ordered-Set Agg #10193

Honeta commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

st1page left a comment

st1page commented Jun 8, 2023 •

edited

Loading

stdrc left a comment

feat(binder): Support bind args in Ordered-Set Agg #10193

feat(binder): Support bind args in Ordered-Set Agg #10193

Conversation

Honeta commented Jun 6, 2023 • edited Loading

What's changed and what's your intention?

Checklist For Contributors

Checklist For Reviewers

Documentation

Types of user-facing changes

Release note

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

st1page left a comment

Choose a reason for hiding this comment

st1page commented Jun 8, 2023 • edited Loading

stdrc left a comment

Choose a reason for hiding this comment

Honeta commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

st1page commented Jun 8, 2023 •

edited

Loading