xuyangzhong commented on pull request #17118:
URL: https://github.com/apache/flink/pull/17118#issuecomment-912210846
@Airblader Hi，thanks to review my immature code, and i want to explain why
the implement function _getExtraDigests_ needs an _Optional_ arg.
------
In the current code, these classes
LimitPushDownSpec/PartitionPushDownSpec/ProjectPushDownSpec/ReadingMetadataSpec/SourceWatermarkSpec
don't need additional information to generate digests by using field in each
SourceAbilitySpec subclass.
However, in classes FilterPushDownSpec and WatermarkPushDownSpec, their
digest should be generated by expressions which are not in these classes. These
classes only have _predicates_(RexNode)(in FilterPushDownSpec),
_watermarkExpr_(RexNode)(in WatermarkPushDownSpec) and
producedType(RowType).
Now we can use _RexNodeToExpressionConverter_ and
_relnode.getExpressionString_ to convert relnode to expression to meet the
digest generation requirement. But:
- _RexNodeToExpressionConverter_ needs _rexbuilder_ and _scan.context_ as
arg in function. Passing these parameters in the constructor in
sourceAbilitySpec subclass will pollute these subclass and lead to bring
additional serialization and deserialization information.
- relNode.getExpressionString is not a static function and needs a relnode
object. The same is that i think bringing a extra relnode into subclass is also
not a good idea.
Based on the above considerations, i think the current good way is to pass
an arg into this function. When we don't need the expression info, the arg is
empty optional. While we meet FilterPushDownSpec, WatermarkPushDownSpec and
other Spec subclass in future, the arg can bring some useful information to
help generate digest.
Look forward your review and reply！:)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
