You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@livy.apache.org by GitBox <gi...@apache.org> on 2020/04/02 01:50:32 UTC
[GitHub] [incubator-livy] wypoon opened a new pull request #288:
[LIVY-754][THRIFT] Encode precision and decimal for decimal type.
wypoon opened a new pull request #288: [LIVY-754][THRIFT] Encode precision and decimal for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288
## What changes were proposed in this pull request?
When a `org.apache.livy.thriftserver.session.DataType` is converted to a `org.apache.hive.service.rpc.thrift.TTypeDesc` for sending a Thrift response to a client request for result set metadata, the `TTypeDesc` contains a `TPrimitiveTypeEntry(TTypeId.DECIMAL_TYPE)` without `TTypeQualifiers` (which are needed to capture the precision and scale).
With this change, we include the qualifiers in the `TPrimitiveTypeEntry`. We use both the name and the `DataType` of a field type to construct the `TTypeDesc`. We are able to do this without changing the existing internal representation for data types because we can obtain the precision and scale from the name of the decimal type.
## How was this patch tested?
Use beeline to connect to the Thrift server. Do a select from a table with a column of decimal type.
Also extended an existing integration test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request
#288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
mgaido91 commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r403271842
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
+ val (precision, scale) =
+ if (name == "decimal") {
+ (10, 0)
+ } else {
+ val suffix = name.substring("decimal".length)
+ require(suffix.startsWith("(") && suffix.endsWith(")"),
+ name + " is not of the form decimal(<precision>,<scale>)")
+ val parts = suffix.substring(1, suffix.length - 1).split(",")
+ if (parts.length == 1) {
+ (parts(0).trim.toInt, 0)
+ } else {
+ (parts(0).trim.toInt, parts(1).trim.toInt)
+ }
Review comment:
nit:
```suggestion
val parts = suffix.substring(1, suffix.length - 1).split(",").map(_.trim.toInt)
(parts(0), parts.lift(1).getOrElse(0))
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r404292676
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
+ val (precision, scale) =
+ if (name == "decimal") {
+ (10, 0)
+ } else {
+ val suffix = name.substring("decimal".length)
+ require(suffix.startsWith("(") && suffix.endsWith(")"),
+ name + " is not of the form decimal(<precision>,<scale>)")
+ val parts = suffix.substring(1, suffix.length - 1).split(",")
+ if (parts.length == 1) {
+ (parts(0).trim.toInt, 0)
+ } else {
+ (parts(0).trim.toInt, parts(1).trim.toInt)
+ }
Review comment:
Your version is more functional. I happen to think my version is easier to understand for less FP-oriented, more old-school people (like myself ;-)). But I have adopted your suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] andrasbeni commented on a change in pull request
#288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
andrasbeni commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402131693
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
+ val (precision, scale) =
+ if (name == "decimal") {
+ (10, 0)
+ } else {
+ val suffix = name.substring(7)
Review comment:
nit: I think replacing `7` with `"decimal".length()` would make this more readable. What's your take on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on issue #288: [LIVY-754][THRIFT]
Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on issue #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-609955560
Added a couple more cases to the integration test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-607587229
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=h1) Report
> Merging [#288](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/ee7fdfc45d90c0478dcd446bc8a19a217eebe04d&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
============================================
+ Coverage 68.19% 68.26% +0.06%
- Complexity 964 965 +1
============================================
Files 104 104
Lines 5952 5952
Branches 900 900
============================================
+ Hits 4059 4063 +4
+ Misses 1314 1310 -4
Partials 579 579
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0.00%> (ø)` | `2.00% <0.00%> (ø%)` | |
| [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `75.00% <0.00%> (+1.25%)` | `40.00% <0.00%> (ø%)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `88.57% <0.00%> (+5.71%)` | `9.00% <0.00%> (+1.00%)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=footer). Last update [ee7fdfc...6f5b968](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402447395
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
Review comment:
```
scala> def f(name: String): (Int, Int) = {
| if (name == "decimal") {
| (10, 0)
| } else {
| val suffix = name.substring(7)
| require(suffix.startsWith("(") && suffix.endsWith(")"),
| name + " is not of the form decimal(<precision>,<scale>)")
| val parts = suffix.substring(1, suffix.length - 1).split(",")
| if (parts.length == 1) {
| (parts(0).trim.toInt, 0)
| } else {
| (parts(0).trim.toInt, parts(1).trim.toInt)
| }
| }
| }
f: (name: String)(Int, Int)
scala> f("decimal")
res0: (Int, Int) = (10,0)
scala> f("decimal(7)")
res1: (Int, Int) = (7,0)
scala> f("decimal(9, 2)")
res2: (Int, Int) = (9,2)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] andrasbeni commented on a change in pull request
#288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
andrasbeni commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402130614
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
Review comment:
Are `decimal` and `decimal(p)` actually possible? I understand these forms can be used to declare the type but based on org.apache.spark.sql.types.DecimalType I don't think the json omits scale or precision.
I might be wrong here. If so, then I believe the parsing logic should be tested for `decimal` and `decimal(p)` also.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-607587229
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=h1) Report
> Merging [#288](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/ee7fdfc45d90c0478dcd446bc8a19a217eebe04d&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
============================================
+ Coverage 68.19% 68.26% +0.06%
- Complexity 964 965 +1
============================================
Files 104 104
Lines 5952 5952
Branches 900 900
============================================
+ Hits 4059 4063 +4
+ Misses 1314 1310 -4
Partials 579 579
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0.00%> (ø)` | `2.00% <0.00%> (ø%)` | |
| [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `75.00% <0.00%> (+1.25%)` | `40.00% <0.00%> (ø%)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `88.57% <0.00%> (+5.71%)` | `9.00% <0.00%> (+1.00%)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=footer). Last update [ee7fdfc...6f5b968](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402443508
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
+ val (precision, scale) =
+ if (name == "decimal") {
+ (10, 0)
+ } else {
+ val suffix = name.substring(7)
Review comment:
Sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io commented on issue #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-607587229
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=h1) Report
> Merging [#288](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/ee7fdfc45d90c0478dcd446bc8a19a217eebe04d&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
============================================
+ Coverage 68.19% 68.26% +0.06%
- Complexity 964 965 +1
============================================
Files 104 104
Lines 5952 5952
Branches 900 900
============================================
+ Hits 4059 4063 +4
+ Misses 1314 1310 -4
Partials 579 579
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0.00%> (ø)` | `2.00% <0.00%> (ø%)` | |
| [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `75.00% <0.00%> (+1.25%)` | `40.00% <0.00%> (ø%)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/288/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `88.57% <0.00%> (+5.71%)` | `9.00% <0.00%> (+1.00%)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=footer). Last update [ee7fdfc...6f5b968](https://codecov.io/gh/apache/incubator-livy/pull/288?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402447395
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
Review comment:
```
scala> def f(name: String): (Int, Int) = {
| if (name == "decimal") {
| (10, 0)
| } else {
| val suffix = name.substring(7)
| require(suffix.startsWith("(") && suffix.endsWith(")"),
| name + " is not of the form decimal(<precision>,<scale>)")
| val parts = suffix.substring(1, suffix.length - 1).split(",")
| if (parts.length == 1) {
| (parts(0).trim.toInt, 0)
| } else {
| (parts(0).trim.toInt, parts(1).trim.toInt)
| }
| }
| }
f: (name: String)(Int, Int)
scala> f("decimal")
res0: (Int, Int) = (10,0)
scala> f("decimal(7)")
res1: (Int, Int) = (7,0)
scala> f("decimal(9, 2)")
res2: (Int, Int) = (9,2)
scala> f("decimal_type")
java.lang.IllegalArgumentException: requirement failed: decimal_type is not of the form decimal(<precision>,<scale>)
at scala.Predef$.require(Predef.scala:224)
at f(<console>:28)
... 49 elided
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402450779
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
Review comment:
I feel that it is overkill to write a unit test just for that block of code. The above suffices.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on a change in pull request #288:
[LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#discussion_r402442829
##########
File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/types/Schema.scala
##########
@@ -109,9 +109,42 @@ object Schema {
case _ => TTypeId.STRING_TYPE
}
val primitiveEntry = new TPrimitiveTypeEntry(typeId)
+ if (dt == DataType.DECIMAL) {
+ val qualifiers = getDecimalQualifiers(name)
+ primitiveEntry.setTypeQualifiers(qualifiers)
+ }
val entry = TTypeEntry.primitiveEntry(primitiveEntry)
val desc = new TTypeDesc
desc.addToTypes(entry)
desc
}
+
+ private def getDecimalQualifiers(name: String): TTypeQualifiers = {
+ // name can be one of
+ // 1. decimal
+ // 2. decimal(p)
+ // 3. decimal(p, s)
Review comment:
In the Hive that I used, I do not actually encounter decimal or decimal(p). I defined a Hive table with columns of each of those variants, and doing a "desc table" returns columns of type decimal(10,0) and decimal(p,0) for the first two variants. In this case, the json that Spark generates and used by `DataTypeUtils.schemaFromSparkJson` only has the third variant.
Nevertheless, I decided to handle all 3 variants purely as a defensive measure. It may be redundant but it doesn't hurt.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] wypoon commented on issue #288: [LIVY-754][THRIFT]
Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on issue #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-608169010
@mgaido91 @jerryshao can you please review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] mgaido91 closed pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
mgaido91 closed pull request #288:
URL: https://github.com/apache/incubator-livy/pull/288
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-livy] wypoon commented on pull request #288: [LIVY-754][THRIFT] Encode precision and scale for decimal type.
Posted by GitBox <gi...@apache.org>.
wypoon commented on pull request #288:
URL: https://github.com/apache/incubator-livy/pull/288#issuecomment-631224025
@mgaido91 can you please merge this (since you have already approved it)?
I thought it was already merged, but it appears that it isn't.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org