KurtYoung edited a comment on issue #7664: [FLINK-11449][table] Uncouple the Expression class from RexNodes. URL: https://github.com/apache/flink/pull/7664#issuecomment-463553997 Thanks @sunjincheng121 for this great effort. I just quickly go through the changes, and have some design comments: 1. It appears to me that i can't tell the differences between `Expression` and `PlannerExpression`. What are the scenarios for both expressions? Take `FilterableTableSource` for example, currently it takes a list of `Expression` for input. But clearly the method `applyPredicate` happens during optimize phase, my feeling is we should use `PlannerExpression` during optimize. (I know `FilterableTableSource` is API and should only accept `Expression` which is also an API, but the logic seems a little weird). 2. What about we just use current `Expression` for API, by getting rid of the `toRexNode` method. The only problem is the number of expressions is quite big and keeps increasing. To solve this, we could follow the advices @twalthr had suggested, we categorize all expressions we currently have, and squash some all them. For example, we can squash "IF", "ABS" into `CallExpression`, and use `FunctionDefinition` to represent details information. Quite similar with Calcite's `RexNode` and `SqlOperator`. And for `ExpressionVisitor`, we can directly mapping `Expression` to `RexNode`, which seems to be much clearer.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
