[GitHub] KurtYoung commented on issue #7664: [FLINK-11449][table] Uncouple the Expression class from RexNodes.

[GitBox <gi...@apache.org>]

KurtYoung commented 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 architecture 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:
users@infra.apache.org


With regards,
Apache Git Services