Skip to content

Conversation

@lmatz
Copy link
Contributor

@lmatz lmatz commented Mar 10, 2022

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

This PR adds a special single_value aggregation function as defined in Calcite.

SINGLE_VALUE aggregate function returns the input value if there
is only one value in the input; Otherwise it triggers a run-time error.`

Its appearance is due to scalar subquery. Scalar subquery, such as in
select .... from .... where column X = (select column Y from ......)
, should only return one row.

single_value aggregation function would check whether the subquery indeed returns exactly one row. The different behaviors from general aggregation functions is:
pub fn single_value<'a, T>(result: Option<T>, input: Option<T>) -> Option<T> now needs to return a result as it cannot accept Some twice.

Based on the observation of PG, it does not really error when there is no row returned. So we don't need to change output_concrete.

Haven't added tests yet.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

@github-actions github-actions bot added the type/feature Type: New feature. label Mar 10, 2022
@TennyZhuang TennyZhuang requested a review from fuyufjh March 10, 2022 08:40
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #824 (a9c6b64) into main (8ce69a3) will increase coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #824      +/-   ##
============================================
+ Coverage     72.22%   72.24%   +0.01%     
  Complexity     2766     2766              
============================================
  Files           923      923              
  Lines         53652    53700      +48     
  Branches       1787     1787              
============================================
+ Hits          38752    38796      +44     
- Misses        14010    14014       +4     
  Partials        890      890              
Flag Coverage Δ
java 61.22% <100.00%> (+<0.01%) ⬆️
rust 76.82% <88.73%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
rust/common/src/expr/agg.rs 40.00% <0.00%> (-4.45%) ⬇️
...ream/src/executor/managed_state/aggregation/mod.rs 80.00% <ø> (ø)
...t/common/src/vector_op/agg/general_distinct_agg.rs 75.65% <77.77%> (+0.16%) ⬆️
rust/common/src/vector_op/agg/functions.rs 75.00% <79.16%> (-2.78%) ⬇️
...m/risingwave/planner/rel/physical/RwAggregate.java 48.38% <100.00%> (+1.72%) ⬆️
rust/common/src/vector_op/agg/aggregator.rs 96.22% <100.00%> (+0.30%) ⬆️
rust/common/src/vector_op/agg/general_agg.rs 97.63% <100.00%> (+0.47%) ⬆️
rust/common/src/types/ordered_float.rs 25.82% <0.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ce69a3...a9c6b64. Read the comment docs.

@lmatz lmatz enabled auto-merge (squash) March 10, 2022 09:39
@lmatz lmatz merged commit 1815835 into main Mar 10, 2022
@lmatz lmatz deleted the lz/single_2 branch March 10, 2022 09:54
Comment on lines +91 to +92
(None, _) => Ok(input),
(Some(_), None) => Ok(result),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a column with [None, Some(2), None] would return Ok rather than Err? It is an error in pg.

btw, how does it work in 2-phase agg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

I believe these two mistakes that need to be corrected. Let me track both points in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants