You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by twalthr <gi...@git.apache.org> on 2015/11/03 15:32:36 UTC
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
GitHub user twalthr opened a pull request:
https://github.com/apache/flink/pull/1322
[FLINK-2961] [table] Add support for basic type Date in Table API
Currently, the basic type Date is not implemented in the Table API. In order to have a mapping of the most important ANSI SQL types for FLINK-2099. It makes sense to add support for Date to represent date, time and timestamps of milliseconds precision. This PR implements FLINK-2961.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twalthr/flink TableApiDate
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1322.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 #1322
----
commit 78d74b2fc03344f9f6798895e32f743d47da6c1d
Author: twalthr <tw...@apache.org>
Date: 2015-11-03T14:18:19Z
[FLINK-2961] [table] Add support for basic type Date in Table API
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-159999863
I have updated my code again. Internally I already implemented a TableConfig that defines the TimeZone and NullCheck. Once #1237 is merged, I will make the config available through TableEnvironment as part of FLINK-3068. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1322
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1322#discussion_r43757052
--- Diff: flink-staging/flink-table/src/main/scala/org/apache/flink/api/table/codegen/ExpressionCodeGenerator.scala ---
@@ -202,6 +210,23 @@ abstract class ExpressionCodeGenerator[R](
""".stripMargin
}
+ case expressions.Literal(dateValue: Date, DATE_TYPE_INFO) =>
+ val dateName = s"""date_${dateValue.getTime}"""
+ val dateStmt = s"""static java.util.Date $dateName
+ |= new java.util.Date(${dateValue.getTime});""".stripMargin
+ reusableStatements.add(dateStmt)
+
+ if (nullCheck) {
+ s"""
+ |val $nullTerm = false
--- End diff --
These are not tested anywhere. We should maybe add tests that also verify the Code Generator when `nullCheck` is `true`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-153421637
It seems that `nullCheck` has never been `true`. The code generator had lots of bugs :D
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #1322: [FLINK-2961] [table] Add support for basic type Date in T...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/flink/pull/1322
[](https://coveralls.io/builds/13591448)
Changes Unknown when pulling **2544011784017fc188882234fe38ebf6b3c58b84 on twalthr:TableApiDate** into ** on apache:master**.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1322#discussion_r43755112
--- Diff: flink-staging/flink-table/src/main/scala/org/apache/flink/api/table/codegen/ExpressionCodeGenerator.scala ---
@@ -202,6 +210,23 @@ abstract class ExpressionCodeGenerator[R](
""".stripMargin
}
+ case expressions.Literal(dateValue: Date, DATE_TYPE_INFO) =>
+ val dateName = s"""date_${dateValue.getTime}"""
+ val dateStmt = s"""static java.util.Date $dateName
+ |= new java.util.Date(${dateValue.getTime});""".stripMargin
+ reusableStatements.add(dateStmt)
+
+ if (nullCheck) {
+ s"""
+ |val $nullTerm = false
--- End diff --
Aren't you generating Java code here? This should not work with `val`. I assume that you don't have a test case where you generate this code. Otherwise it should have failed. I guess it would be good to add a test case for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-153395500
I also also thought about that but I'm pretty sure that this introduces issues difficult to debug. I think the most important thing is that the conversion (from string to date and back) is always constant. Internally date is always UTC anyway. Introducing a `.convert()` function for that might be better. My goal was primarily to have this data type in the Table API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/1322#discussion_r43766940
--- Diff: flink-staging/flink-table/src/main/scala/org/apache/flink/api/table/codegen/ExpressionCodeGenerator.scala ---
@@ -202,6 +210,23 @@ abstract class ExpressionCodeGenerator[R](
""".stripMargin
}
+ case expressions.Literal(dateValue: Date, DATE_TYPE_INFO) =>
+ val dateName = s"""date_${dateValue.getTime}"""
+ val dateStmt = s"""static java.util.Date $dateName
+ |= new java.util.Date(${dateValue.getTime});""".stripMargin
+ reusableStatements.add(dateStmt)
+
+ if (nullCheck) {
+ s"""
+ |val $nullTerm = false
--- End diff --
Good point. `val` occurs in all literal code generation lines. I copy pasted the bug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-160129102
Looks good now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-160358029
I will merge this later today, if nobody has objections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-154508639
I think the best solution (also for future features like the SQL API) is a TableConfig, where TimeZone, Locales, and also the `nullCheck` can be configured. Once #1237 is merged (hopefully soon), I will implement that into the `TableEnvironment`. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2961] [table] Add support for basic typ...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1322#issuecomment-153385795
So all timestamps have to be in UTC timezone. Isn't this a bit limiting? Could it per default take the system timezone and allow to override the timezone in a cast statement?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---