Codecov Report

Merging #854 (8e2a3c2) into main (9f2387b) will decrease coverage by 0.05%.
The diff coverage is 78.94%.

@@             Coverage Diff              @@
##               main     #854      +/-   ##
============================================
- Coverage     72.02%   71.96%   -0.06%     
  Complexity     2766     2766              
============================================
  Files           927      927              
  Lines         54086    54139      +53     
  Branches       1787     1787              
============================================
+ Hits          38953    38960       +7     
- Misses        14243    14289      +46     
  Partials        890      890              

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

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

As for 2-phase agg, the sub-counts from local results needs to be summed. An immature idea is to rewrite it like [localCount,first] - [globalSum,first] - Project[ globalSum <= 1 ? first : err ], just like avg is rewritten to [localCount, localSum] - [globalSum0, globalSum1] - Project [ globalSum1 / globalSum0 ]

What about impl Aggregator rather than reusing GeneralAgg? The extension in this PR means some state are captured in the GeneralAgg::result, and some in GeneralAgg::f. Can we keep f pure and capture all states explicitly in a struct that impl Aggregator?

Let me try it 👌 .

As for 2-phase agg, the sub-counts from local results needs to be summed. An immature idea is to rewrite it like [localCount,first] - [globalSum,first] - Project[ globalSum <= 1 ? first : err ], just like avg is rewritten to [localCount, localSum] - [globalSum0, globalSum1] - Project [ globalSum1 / globalSum0 ]

You are suggesting this can be done purely in the frontend, do I get your point?
Once a local single_value aggregation has a count > 1, then the entire query should stop and return a runtime error as PG does. We can error earlier, i.e. at the local stage, instead of erroring later, i.e. at the global stage? This could be the only reason I think rewriting may be worse in terms of performance, but very close as the number of messages needed to be exchanged is small. (but may also be large as first still needs to be called a lot of times if the query is ill-written?)

Let's hold this PR for a while and seek other ones' opinion.

@liurenjie1024 @fuyufjh @TennyZhuang @st1page What do you guys think?

As for 2-phase agg, the sub-counts from local results needs to be summed. An immature idea is to rewrite it like [localCount,first] - [globalSum,first] - Project[ globalSum <= 1 ? first : err ], just like avg is rewritten to [localCount, localSum] - [globalSum0, globalSum1] - Project [ globalSum1 / globalSum0 ]

You are suggesting this can be done purely in the frontend, do I get your point? Once a local single_value aggregation has a count > 1, then the entire query should stop and return a runtime error as PG does. We can error earlier, i.e. at the local stage, instead of erroring later, i.e. at the global stage? This could be the only reason I think rewriting may be worse in terms of performance, but very close as the number of messages needed to be exchanged is small.

Yes, it is mostly frontend. I also prefer to error out earlier at local stage, but unsure if it is always possible. What if there is 1 null at each local node? The local node cannot call it an error yet, and the global one cannot differentiate this case with a correct empty result, without getting count from local.

There may be better solutions. Just to complete the idea above, frontend rewrite will remove this special aggregate function, but it still need backend support for first and error reporting in Project. Actually first can be any or even min, but min runs slower and any is less useful.

As @fuyufjh suggests, there could be a more general solution that uses other operators, i.e. join, to achieve this instead of a special single_value aggregation function. Since we want to run the streaming tpch q15 query now, we may still need this aggregation function as a temporary workaround. Therefore, the related issue post will be kept.

Since we may remove this altogether later, the PR keeps the current implementation for the moment. And this issue will be investigated further.