flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kmurra <...@git.apache.org>
Subject [GitHub] flink pull request #4746: [FLINK-7657] [Table] Adding logic to convert RexLi...
Date Wed, 04 Oct 2017 21:45:11 GMT
Github user kmurra commented on a diff in the pull request:

    https://github.com/apache/flink/pull/4746#discussion_r142801313
  
    --- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/literals.scala
---
    @@ -49,10 +50,51 @@ object Literal {
         case sqlTime: Time => Literal(sqlTime, SqlTimeTypeInfo.TIME)
         case sqlTimestamp: Timestamp => Literal(sqlTimestamp, SqlTimeTypeInfo.TIMESTAMP)
       }
    +
    +  private[flink] def apply(rexNode: RexLiteral): Literal = {
    --- End diff --
    
    I'll commit to using your standards for the code-base.  However, allow me to voice a disagreement
here:
    
    The Literal class does the conversion from the Literal back to the RexLiteral.  Having
this logic specifically in RexNodeToExpressionConverted means the RexLiteral-to-Literal logic
is physically split from the Literal-to-RexLiteral logic.  This makes it slightly easier for
a contributor to make a change in one side of the conversion without accounting for the other.
 In particular, the date adjustments here become harder to understand since the context is
split between two different files. 



---

Mime
View raw message