[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/flink/pull/6252
  
@fhueske 
Ah I didn't notice the object is already existing. Thanks for letting me 
know!


---

[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/6252
  
I think it can also be a new public method in 
`org.apache.flink.table.expressions.ExpressionUtils`.

Thanks, Fabian


---

[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/flink/pull/6252
  
@fhueske 
Yeah, that would be great. I'm happy to revert the change and introduce new 
class. 

I'm just not sure where to place and how to name it, since it will be going 
to have just one method, `getReturnType(Expression): TypeInformation[_]`.

Would `org.apache.flink.table.api.ExpressionUtil` be OK for new utility 
class?

Thanks in advance!


---

[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/6252
  
Thanks for opening this PR @HeartSaVioR! (cc @twalthr)

I thought about this again and I don't think we should make the method 
public. The problem is that `Expression` is one of the core classes of the 
Table API. By making the method public it becomes visible to all users of the 
API.

A better approach is to provide a publicly accessible util object (in 
org.apache.flink.table.api...) that provides access to the result type (and 
possibly other properties) of ab `Expression`.

What do you think? 

Best, Fabian


---