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 2016/04/13 16:24:26 UTC
[GitHub] flink pull request: [FLINK-3749] [table] Improve decimal handling
GitHub user twalthr opened a pull request:
https://github.com/apache/flink/pull/1881
[FLINK-3749] [table] Improve decimal handling
This PR improves the decimal handling and fixes several bugs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twalthr/flink TableApiDecimalFix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1881.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 #1881
----
commit ba57e784d2e251b2b1b275724fe039f82789ff4e
Author: twalthr <tw...@apache.org>
Date: 2016-04-13T14:17:07Z
[FLINK-3749] [table] Improve decimal handling
----
---
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-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-210000094
Yes, you are right. We need special data type etc. for that in the future. But until then we should treat it as double as long as we can represent it. I can create an issue for that.
---
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-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-211892621
@vasia and @fhueske: I have opened FLINK-3786 for a long-term solution ;-)
I will start working on this issue.
---
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-3749] [table] Improve decimal handling
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-210007776
> There is nothing more permanent than than a temporary solution ;-)
Please correct me, if I got something wrong.
This change adds support for `DECIMAL` literals. So we might have row fields of `DECIMAL` type and might compute incorrect results until we add proper support for `DECIMAL`.
It would be nice to support `DECIMAL` but I don't think it is more important than result correctness. Depending on the support Calcite (or other libraries) provide, adding proper `DECIMAL` support might be a complex task and might take some time.
Is there an urgent need to support `DECIMAL`?
---
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 #1881: [FLINK-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr closed the pull request at:
https://github.com/apache/flink/pull/1881
---
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-3749] [table] Improve decimal handling
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-211358495
@twalthr I see your point. Is it possible to support at least decimals of double precision without running into the risk of returning wrong results? If not, I don't think we should go for this temporary solution.
---
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-3749] [table] Improve decimal handling
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-209985156
I don't think we should handle `DECIMAL` as `double`. `DECIMAL` provides fixed precision in contrast to `double`. If we internally represent `DECIMAL` as `double` precision is not guaranteed. I think we need a special data type, serializers, and also custom math logic to handle `DECIMAL` correctly. Maybe Calcite offers some of that?
---
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-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-210355092
> It's better to have a temporary solution than no solution ;-)
Calcite translates all decimal literals to `DECIMAL`. At the moment even `SELECT 11.2 FROM MyTable` does not work, although it could fit into `DOUBLE`. I thought it makes sense to support at least decimals of `DOUBLE` precision.
---
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-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-212434847
Solving FLINK-3786 should not be too complicated. Integrating it into the Table API might need some time.
---
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-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/1881#discussion_r59699195
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/codegen/CodeGenerator.scala ---
@@ -551,23 +551,27 @@ class CodeGenerator(
case BIGINT =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
if (decimal.isValidLong) {
- generateNonNullLiteral(resultType, decimal.longValue().toString)
+ generateNonNullLiteral(resultType, decimal.longValue().toString + "L")
}
else {
throw new CodeGenException("Decimal can not be converted to long.")
}
case FLOAT =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
- if (decimal.isValidFloat) {
- generateNonNullLiteral(resultType, decimal.floatValue().toString + "f")
+ // check if we loose/change digits when converting to float
+ val converted = BigDecimal(decimal.floatValue().toString)
+ if (converted == decimal) {
+ generateNonNullLiteral(resultType, converted.toString + "f")
}
else {
throw new CodeGenException("Decimal can not be converted to float.")
}
- case DOUBLE =>
+ case DOUBLE | DECIMAL =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
- if (decimal.isValidDouble) {
- generateNonNullLiteral(resultType, decimal.doubleValue().toString)
+ // check if we loose/change digits when converting to double
+ val converted = BigDecimal(decimal.doubleValue().toString)
--- End diff --
No, thats a different issue. The `ExpressionParser` needs a large rework for all that.
---
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-3749] [table] Improve decimal handling
Posted by ramkrish86 <gi...@git.apache.org>.
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1881#discussion_r59661849
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/codegen/CodeGenerator.scala ---
@@ -551,23 +551,27 @@ class CodeGenerator(
case BIGINT =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
if (decimal.isValidLong) {
- generateNonNullLiteral(resultType, decimal.longValue().toString)
+ generateNonNullLiteral(resultType, decimal.longValue().toString + "L")
}
else {
throw new CodeGenException("Decimal can not be converted to long.")
}
case FLOAT =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
- if (decimal.isValidFloat) {
- generateNonNullLiteral(resultType, decimal.floatValue().toString + "f")
+ // check if we loose/change digits when converting to float
+ val converted = BigDecimal(decimal.floatValue().toString)
+ if (converted == decimal) {
+ generateNonNullLiteral(resultType, converted.toString + "f")
}
else {
throw new CodeGenException("Decimal can not be converted to float.")
}
- case DOUBLE =>
+ case DOUBLE | DECIMAL =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
- if (decimal.isValidDouble) {
- generateNonNullLiteral(resultType, decimal.doubleValue().toString)
+ // check if we loose/change digits when converting to double
+ val converted = BigDecimal(decimal.doubleValue().toString)
--- End diff --
So with this change all the 11.CAST(STRING) and 11.abs() all should work right?
---
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 #1881: [FLINK-3749] [table] Improve decimal handling
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/1881
Yes, I will close this...
---
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-3749] [table] Improve decimal handling
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1881#issuecomment-212375931
Thanks @twalthr !
Do you think solving FLINK-3786 will be a lot of effort?
I did not know that Calcite internally handles all fraction literals as DECIMAL. That explains the need for this data type.
---
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 #1881: [FLINK-3749] [table] Improve decimal handling
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/1881
@twalthr, can this PR be closed in favor of #2088?
---
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.
---