Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(agg,over_window): support first_value, last_value and refactor LogicalAgg #10740

Merged
merged 19 commits into from Jul 5, 2023

Conversation

stdrc
Copy link
Contributor

@stdrc stdrc commented Jul 4, 2023

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

What's changed and what's your intention?

  • Support last_value aggregate function and hence corresponding window function
  • Expose first_value and last_value to user
  • Refactor LogicalAgg to make match arms clean and also allow more 2-phase agg kind

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.

  • SQL commands, functions, and operators

Release note

  • Support first_value and last_value aggregate function and window function

stdrc added 7 commits July 4, 2023 18:04
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
@github-actions github-actions bot added type/feature user-facing-changes Contains changes that are visible to users labels Jul 4, 2023
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
@kwannoel kwannoel changed the title feat(agg,over_window): support last_value and refactor LogicalAgg feat(agg,over_window): support first_value, last_value and refactor LogicalAgg Jul 5, 2023
Copy link
Contributor

@kwannoel kwannoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sqlsmith part LGTM

@kwannoel
Copy link
Contributor

kwannoel commented Jul 5, 2023

Looks there might be some bugs introduced though, see fuzzing-1.log in build artifacts - https://buildkite.com/risingwavelabs/pull-request/builds/26777#01892404-766d-4c48-a47f-49224f2ee6e0:

2022-10-11T05:44:26.555094Z  INFO node{id=7 name="frontend-1"}:task{id=561}:run_stmt{stmt=CREATE MATERIALIZED VIEW m14 AS SELECT approx_count_distinct('5D1ZlMzATx') AS col_0 FROM m8 AS t_0 WHERE true GROUP BY t_0.col_0, t_0.col_2 HAVING true session_id=0}: risingwave_expr::sig::agg: 211 aggregations loaded.
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `3`', src/meta/src/stream/stream_graph/fragment.rs:279:9
stack backtrace:
CREATE MATERIALIZED VIEW m14 AS
SELECT
    approx_count_distinct ('5D1ZlMzATx') AS col_0
FROM
    m8 AS t_0
WHERE
    TRUE
GROUP BY
    t_0.col_0,
    t_0.col_2
HAVING
    TRUE

| AggKind::JsonbObjectAgg
| AggKind::PercentileCont
| AggKind::PercentileDisc
| AggKind::Mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't approx count distinct fall under this category? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And StringAgg too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be the source of errors.

Copy link
Contributor Author

@stdrc stdrc Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the src/meta/src/stream/stream_graph/fragment.rs:279:9 is because visit_stream_node_tables_inner didn't correctly visit state table of DistinctApproxCount, will dive in later. While OTOH 2-phase sum0(hyperloglog) may indeed not equal to hyperloglog, not quite sure. I'll add it to this category.

For StringAgg, I was thinking that when user doesn't provide ORDER BY, the call can actually be 2-phased by just concatenating strings in arbitrary nondeterministic order.

Copy link
Contributor

@kwannoel kwannoel Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For StringAgg, I was thinking that when user doesn't provide ORDER BY, the call can actually be 2-phased by just concatenating strings in arbitrary nondeterministic order.

Agree on correctness, although I cant think of a usecase.
I suppose there's some other bug around it then. Maybe it is executing in non-append-only. We should add another check for that.

An e2e test will be great to test two-phase string_agg as well, I don't think it is currently tested. Same goes for any other two-phase agg we have just started supporting in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems 2-phase StringAgg has other problem, that is the final phase can't get delim argument from input. Maybe better to disable it again for now😇.

stdrc added 6 commits July 5, 2023 12:49
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
/// Macros to generate match arms for [`AggKind`].
/// IMPORTANT: These macros must be carefully maintained especially when adding new [`AggKind`]
/// variants.
pub(crate) mod agg_kinds {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can infer or allow developers to specify these properties in the #[aggregate] macro later. 🤔

stdrc added 4 commits July 5, 2023 13:32
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #10740 (d6e9ffb) into main (2732fef) will increase coverage by 0.00%.
The diff coverage is 65.24%.

@@           Coverage Diff           @@
##             main   #10740   +/-   ##
=======================================
  Coverage   70.14%   70.15%           
=======================================
  Files        1296     1296           
  Lines      221698   221748   +50     
=======================================
+ Hits       155516   155569   +53     
+ Misses      66182    66179    -3     
Flag Coverage Δ
rust 70.15% <65.24%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/expr/src/agg/def.rs 67.96% <0.00%> (-0.67%) ⬇️
...c/frontend/src/optimizer/rule/distinct_agg_rule.rs 98.76% <ø> (ø)
src/common/src/util/stream_graph_visitor.rs 51.53% <16.66%> (-1.97%) ⬇️
src/expr/src/agg/general.rs 85.26% <20.00%> (-0.86%) ⬇️
...rc/frontend/src/optimizer/plan_node/generic/agg.rs 84.27% <63.01%> (-0.95%) ⬇️
src/stream/src/executor/aggregation/minput.rs 95.80% <87.50%> (-0.09%) ⬇️
src/common/src/util/sort_util.rs 96.34% <100.00%> (+1.55%) ⬆️
src/frontend/src/expr/agg_call.rs 74.16% <100.00%> (+0.21%) ⬆️
...rc/frontend/src/optimizer/plan_node/logical_agg.rs 96.65% <100.00%> (-0.02%) ⬇️

... and 12 files with indirect coverage changes

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

@st1page st1page added this pull request to the merge queue Jul 5, 2023
Merged via the queue into main with commit aea2da0 Jul 5, 2023
36 checks passed
@st1page st1page deleted the rc/agg-last-value branch July 5, 2023 10:01
@emile-00 emile-00 added the 📖✓ Covered or will be covered in the user docs. label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users 📖✓ Covered or will be covered in the user docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants