[GitHub] flink issue #6253: [FLINK-8094][Table API & SQL] Support other types for Exi...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6253 @xccui Yes. Without defining expression with custom reserved keyword, we may need to leverage function which would be non-UDF which means we need to add the function to default provided function list from either Flink or Calcite. ---
[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Expose Expression.resultTyp...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6252 Addressed. ---
[GitHub] flink pull request #6252: [FLINK-9742][Table API & SQL] Expose Expression.re...
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.
---
[GitHub] flink issue #6253: [FLINK-8094][Table API & SQL] Support other types for Exi...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6253 Btw, looks like it may be also possible for ExistingField to replace needs of ParsingExistingField, via adding optional dateformat parameter (not sure which is more preferred), but I feel we can address it later out of this PR. ---
[GitHub] flink issue #6253: [FLINK-8094][Table API & SQL] Support other types for Exi...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6253 @fhueske Thanks for the nice feedback! Addressed your comment except regarding unit test. Let me try out adding unit test later, since it might bring too high barrier as of now. ---
[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Expose Expression.resultTyp...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6252 @fhueske Addressed. Please take a look again. ---
[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6252 @fhueske Ah I didn't notice the object is already existing. Thanks for letting me know! ---
[GitHub] flink issue #6252: [FLINK-9742][Table API & SQL] Widen scope of Expression.r...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6252 @fhueske Yeah, that would be great. I'm happy to revert the change and introduce new class. I'm just not sure where to place and how to name it, since it will be going to have just one method, `getReturnType(Expression): TypeInformation[_]`. Would `org.apache.flink.table.api.ExpressionUtil` be OK for new utility class? Thanks in advance! ---
[GitHub] flink issue #6253: [WIP][FLINK-8094][Table API & SQL] Support other types fo...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6253 If we would like to answer 1 as "Yes, apply the change to ExistingField.", we can rebase this PR with below branch. https://github.com/HeartSaVioR/flink/commits/FLINK-8094-fixing-ExistingField Thanks to @xccui I can copy the test code from his POC commit in above branch. https://github.com/xccui/flink/commit/afcc5f1a0ad92db08294199e61be5df72c1514f8 ---
[GitHub] flink issue #6256: [MINOR] Add generated avro java codes to RAT exclusion li...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6256 @zentol Addressed review comment. ---
[GitHub] flink issue #6256: [MINOR] Add generated avro java codes to .ignore to avoid...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6256 @zentol First of all, thanks for the reviewing! The RAT plugin `reads and parses` the .gitignore file like below so adding it to .gitignore file in root directory is effective. The issue was that it doesn't apply to .gitignore in each module. ``` [INFO] --- apache-rat-plugin:0.12:check (default) @ flink-parent --- [INFO] Added 1 additional default licenses. [INFO] Enabled default license matchers. [INFO] Added 1 custom approved licenses. [INFO] Will parse SCM ignores for exclusions... [INFO] Parsing exclusions from /Users/jlim/WorkArea/JavaProjects/flink/.gitignore [INFO] Finished adding exclusions from SCM ignore files. [INFO] 93 implicit excludes (use -debug for more details). ``` But yes, I agree RAT exclusions are better to set in the plugin configuration. I will update. ---
[GitHub] flink issue #6256: [MINOR] Add generated avro java codes to .ignore to avoid...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6256 There's already .gitignore file in flink-confluent-schema-registry module directory, but it doesn't look like effective while running `mvn clean verify` in flink root directory. ---
[GitHub] flink pull request #6256: [MINOR] Add generated avro java codes to .ignore t...
GitHub user HeartSaVioR opened a pull request: https://github.com/apache/flink/pull/6256 [MINOR] Add generated avro java codes to .ignore to avoid RAT error ## What is the purpose of the change This patch adds generated avro java codes to .ignore to avoid RAT error when running `mvn clean verify` in flink root directory. ## Brief change log * Add generated avro java code to .ignore to avoid RAT error ## Verifying this change This change is already covered by existing tests, such as *mvn clean verify*. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) You can merge this pull request into a Git repository by running: $ git pull https://github.com/HeartSaVioR/flink MINOR-add-generated-classes-to-gitignore Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/6256.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #6256 commit b63119c714e006d71f990fbc889c32fda6fb92a0 Author: Jungtaek Lim Date: 2018-07-04T15:32:32Z [MINOR] Add generated avro java code to .ignore to avoid RAT error ---
[GitHub] flink issue #6253: [WIP][FLINK-8094][Table API & SQL] Support other types fo...
Github user HeartSaVioR commented on the issue: https://github.com/apache/flink/pull/6253 Seeking for some guides on this PR: 1. Maybe we could replace the implementation of ExistingField with the implementation of IsoDateStringAwareExistingField since the implementation is on top of ExistingField. Would we think replacing is better? 2. ExistingField has no test so I also don't add test for IsoDateStringAwareExistingField: I guess adding test might require more understanding of Table API. Would it be OK to skip adding test for IsoDateStringAwareExistingField? cc. @fhueske ---
[GitHub] flink pull request #6253: [FLINK-8094][Table API & SQL] Support other types ...
GitHub user HeartSaVioR opened a pull request: https://github.com/apache/flink/pull/6253 [FLINK-8094][Table API & SQL] Support other types for ExistingField rowtime extractor ## What is the purpose of the change This patch proposes new rowtime extractor which is improved version of ExistingField, handles ISO dateformatted String type as well as Long and Timestamp types. ## Brief change log * introduce IsoDateStringAwareExistingField class to provide improved version of ExistingField * handles ISO dateformatted String type as well as Long and Timestamp types. ## Verifying this change This change added tests and can be verified as follows: - Manually verified the change by applying to TableSource which has ISO dateformatted STRING as rowtime field's type and confirmed watermark working correctly. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (yes) - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) You can merge this pull request into a Git repository by running: $ git pull https://github.com/HeartSaVioR/flink FLINK-8094 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/6253.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #6253 commit 75426f7c2adc18367eab961ce76612b31c837eb0 Author: Jungtaek Lim Date: 2018-07-04T14:24:29Z [FLINK-8094][Table API & SQL] Support other types for ExistingField rowtime extractor This patch proposes new rowtime extractor which is improved version of ExistingField, handles ISO dateformatted String type as well as Long and Timestamp types. ---
[GitHub] flink pull request #6252: [FLINK-9742][Table API & SQL] Widen scope of Expre...
GitHub user HeartSaVioR opened a pull request: https://github.com/apache/flink/pull/6252 [FLINK-9742][Table API & SQL] Widen scope of Expression.resultType to 'public' ## What is the purpose of the change This patch proposes widen scope of Expression.resultType to 'public', to enable custom TimestampExtractor outside of Flink acessing Expression.resultType. There is some use cases of TableSource which requires custom implementation of TimestampExtractor, and 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". It would be better to just make Expression.resultType public to cover other cases as well. Please refer https://issues.apache.org/jira/browse/FLINK-9742 for more details. ## Brief change log * widen scope of Expression.resultType to 'public' * also change the scope of resultType from all subclasses ## Verifying this change This change is a trivial rework / code cleanup without any test coverage. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: no - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) You can merge this pull request into a Git repository by running: $ git pull https://github.com/HeartSaVioR/flink FLINK-9742 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/6252.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #6252 commit abb3c965febb5c5b4110b68c59b509fcf9747d38 Author: Jungtaek Lim Date: 2018-07-04T13:51:45Z FLINK-9742 Widen scope of Expression.resultType to 'public' Widen scope of Expression.resultType to 'public', to enable custom TimestampExtractor outside of Flink acessing Expression.resultType. ---
