[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16534075#comment-16534075
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6252
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533842#comment-16533842
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/6252
Thanks for the update @HeartSaVioR.
I'll merge this
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533659#comment-16533659
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user HeartSaVioR commented on the issue:
https://github.com/apache/flink/pull/6252
Addressed.
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533654#comment-16533654
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/flink/pull/6252#discussion_r200352236
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/ExpressionUtils.scala
---
@@ -22,13 +22,16 @@ import java.lang.{Boolean => JBoolean, Byte => JByte,
Double => JDouble, Float =
import java.math.{BigDecimal => JBigDecimal}
import java.sql.{Date, Time, Timestamp}
-import org.apache.flink.api.common.typeinfo.BasicTypeInfo
+import org.apache.flink.api.common.typeinfo.{BasicTypeInfo,
TypeInformation}
import org.apache.flink.streaming.api.windowing.time.{Time => FlinkTime}
import org.apache.flink.table.api.ValidationException
import org.apache.flink.table.calcite.FlinkTypeFactory
import org.apache.flink.table.typeutils.{RowIntervalTypeInfo,
TimeIntervalTypeInfo}
object ExpressionUtils {
+ def getReturnType(expr: Expression): TypeInformation[_] = {
--- End diff --
Yeah my bad. That's a silly mistake. Will fix the name and add scala docs.
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533627#comment-16533627
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/6252
Looks good. Just one last comment.
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533626#comment-16533626
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/6252#discussion_r200345063
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/ExpressionUtils.scala
---
@@ -22,13 +22,16 @@ import java.lang.{Boolean => JBoolean, Byte => JByte,
Double => JDouble, Float =
import java.math.{BigDecimal => JBigDecimal}
import java.sql.{Date, Time, Timestamp}
-import org.apache.flink.api.common.typeinfo.BasicTypeInfo
+import org.apache.flink.api.common.typeinfo.{BasicTypeInfo,
TypeInformation}
import org.apache.flink.streaming.api.windowing.time.{Time => FlinkTime}
import org.apache.flink.table.api.ValidationException
import org.apache.flink.table.calcite.FlinkTypeFactory
import org.apache.flink.table.typeutils.{RowIntervalTypeInfo,
TimeIntervalTypeInfo}
object ExpressionUtils {
+ def getReturnType(expr: Expression): TypeInformation[_] = {
--- End diff --
Please add Scala docs since this is a public method.
Rename to `getResultType` for consistency?
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-9742) Expose Expression.resultType to public
[
https://issues.apache.org/jira/browse/FLINK-9742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16533623#comment-16533623
]
ASF GitHub Bot commented on FLINK-9742:
---
Github user HeartSaVioR commented on the issue:
https://github.com/apache/flink/pull/6252
@fhueske Addressed. Please take a look again.
> Expose Expression.resultType to public
> --
>
> Key: FLINK-9742
> URL: https://issues.apache.org/jira/browse/FLINK-9742
> Project: Flink
> Issue Type: Improvement
> Components: Table API SQL
>Affects Versions: 1.5.0
>Reporter: Jungtaek Lim
>Priority: Major
> Labels: pull-request-available
>
> I have use case of TableSource which requires custom implementation of
> TimestampExtractor. To ensure new TimestampExtractor to cover more general
> use cases, accessing Expression.resultType is necessary, but its scope is now
> defined as package private for "org.apache.flink".
> Below is the implementation of custom TimestampExtractor which leverages
> Expression.resultType, hence had to place it to org.apache.flink package
> (looks like a hack).
> {code:java}
> class IsoDateStringAwareExistingField(val field: String) extends
> TimestampExtractor {
> override def getArgumentFields: Array[String] = Array(field)
> override def validateArgumentFields(argumentFieldTypes:
> Array[TypeInformation[_]]): Unit = {
> val fieldType = argumentFieldTypes(0)
> fieldType match {
> case Types.LONG => // OK
> case Types.SQL_TIMESTAMP => // OK
> case Types.STRING => // OK
> case _: TypeInformation[_] =>
> throw ValidationException(
> s"Field '$field' must be of type Long or Timestamp or String but is
> of type $fieldType.")
> }
> }
> override def getExpression(fieldAccesses: Array[ResolvedFieldReference]):
> Expression = {
> val fieldAccess: Expression = fieldAccesses(0)
> fieldAccess.resultType match {
> case Types.LONG =>
> // access LONG field
> fieldAccess
> case Types.SQL_TIMESTAMP =>
> // cast timestamp to long
> Cast(fieldAccess, Types.LONG)
> case Types.STRING =>
> Cast(Cast(fieldAccess, SqlTimeTypeInfo.TIMESTAMP), Types.LONG)
> }
> }
> }{code}
> It would be better to just make Expression.resultType public to cover other
> cases as well. (I'm not sure other methods would be also better to be public
> as well.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
