Tartarus0zm commented on pull request #15341: URL: https://github.com/apache/flink/pull/15341#issuecomment-826330825
> > > @Tartarus0zm Thanks for contribution. I have two problems: > > > > > > 1. If I understand correctly, the root cause of the exception is it's not safe assume the top `RelNode` of `RelBuilder` is `Aggregate` after call `RelBuilder`#aggregate. I found there are also other class make the same mistake: > > > > > > * Line 162 in `FlinkRelBuilder`#windowAggregate > > > * Line 286 in `SplitAggregateRule`#onMatch to set PartialFinalType.PARTIAL > > > * Line 314 in `SplitAggregateRule`#onMatch to set PartialFinalType.FINAL > > > It's better to take all those places into consideration. > > > 2. If we only want to support two cases after call `RelBuilder`#aggregate in `SplitAggregateRule` currently: 1. top `RelNode` is project 2. top `RelNode` is aggregate. I think it's better to throw a `TableException` for other pattern in default behavior. > > > > > > @Tartarus0zm Thanks for your effort, LGTM. > > @godfreyhe We need pay attention to the 1st point mentioned above to avoid more unexpected exception. > > @Tartarus0zm could you check the above cases and add some tests for window aggregate which pattern likes the tests you added. I have done a test on window Aggregate, I think this problem also exists. I created a new issue [FLINK-22455](https://issues.apache.org/jira/browse/FLINK-22455) to track it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
