JingsongLi edited a comment on issue #8435: [FLINK-12443][table-planner-blink] Replace InternalType with LogicalType in blink URL: https://github.com/apache/flink/pull/8435#issuecomment-495451358 Hi @aljoscha >I can't really comment too much, also because I think this is probably too big to have a sensible review. Sorry to be such a big PR. I tried to split it up, but it brought a lot of transformations of `InternalType` and `LogicalType` to ensure that every commit was compiled and passed, and finally I chose one PR. >One thing I noticed, though, is that this adds conversion logic in the Blink Planner code. @twalthr recently added code for that (https://github.com/apache/flink/blob/master/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/utils/TypeConversions.java) and I think more conversion code is coming up. We shouldn't have code duplication here. Thank you for reminding me about the `TypeConvertions`. I took a quick look to it, But I think it's different from Blink's conversion right now. 1.Blink should deal with not nullable. (`TypeConvertions` now just deal with nullable true) 2.Blink's conversion will lose some information. Just like PojoTypeInfo, Blink just convert it to RowType, but `TypeConvertions` use `LegacyTypeInformationType` to support it. 3.We can't convert `LogicalType` to `DataType` directly, because we don't have conversion class, but `fromDataTypeToLegacyInfo` need it. Yeah, we shouldn't have code duplication here. I will reuse `TypeConvertions`, but in order to avoid further enlarged changes, I need to make some additional convertions based on it. >Another thing I noticed (you're probably already aware of this), is that in some places you convert logical types to TypeInformation only to then convert this back to logical types. For example AggFunctionFactory.scala has: Good catch, there are still many places where we shouldn't use `TypeInformation`, and I should change them all to use `LogicalType` in follow up PRs.
---------------------------------------------------------------- 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] With regards, Apache Git Services
