[GitHub] [flink] aljoscha commented on issue #8435: [FLINK-12443][table-planner-blink] Replace InternalType with LogicalType in blink

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

aljoscha commented on issue #8435: [FLINK-12443][table-planner-blink] Replace InternalType with LogicalType in blink
URL: https://github.com/apache/flink/pull/8435#issuecomment-495213374
 
 
   I can't really comment too much, also because I think this is probably too big to have a sensible review.
   
   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.
   
   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:
   
   ```
   case DECIMAL =>
     val d = argTypes(0).asInstanceOf[DecimalType]
     val decimalTypeInfo = DecimalTypeInfo.of(d.getPrecision, d.getScale)
     new AvgAggFunction.DecimalAvgAggFunction(decimalTypeInfo)
   ```
   
   and then `DecimalAvgAggFunction` converts this back:
   
   ```
   @Override
   public DataType getResultType() {
   	DecimalType t = FlinkTypeSystem.inferAggAvgType(type.scale());
   	return DataTypes.DECIMAL(t.getPrecision(), t.getScale());
   }
   ```
   
   This is already in the existing code, not introduced in this PR. I just noticed it now, though.

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


With regards,
Apache Git Services