Skip to content

Conversation

fhueske
Copy link
Contributor

@fhueske fhueske commented May 20, 2016

PR fixes DataSetAggregateRule to not translate LogicalAggregate operators that include distinct aggregates or grouping sets. Such aggregations are currently not supported by DataSetAggregate.

  • General
    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation
    • Bug fix does not require docs
  • Tests & Build
    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed

agg.getInput.getRowType,
agg.getGroupSet.toArray)
}
override def matches(call: RelOptRuleCall): Boolean = {
Copy link
Member

@yjshen yjshen May 23, 2016

Choose a reason for hiding this comment

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

Is that possible we add an UnsupportedOperatorRule to match unsupported plan and throws an TableException in convert method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I won't work for all unsupported operators (inner equi-joins are initially Cartesian products + filters before translation rules merge join and filter), but some operations such as distinct aggregates are not rewritten be rules such that we can throw an exception when they are observed.

@fhueske
Copy link
Contributor Author

fhueske commented May 23, 2016

Thanks for the review @yjshen!
Will address the comment and merge this PR.

@asfgit asfgit closed this in 173d24d May 23, 2016
@fhueske fhueske deleted the tableDistAggs branch May 23, 2016 15:29
mbode pushed a commit to mbode/flink that referenced this pull request May 27, 2016
hequn8128 pushed a commit to hequn8128/flink that referenced this pull request Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants