flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] aljoscha commented on issue #8435: [FLINK-12443][table-planner-blink] Replace InternalType with LogicalType in blink
Date Thu, 23 May 2019 13:15:32 GMT
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:
   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,

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:

With regards,
Apache Git Services

View raw message