You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2014/11/11 18:20:18 UTC
[GitHub] spark pull request: [SQL][Minor] Let BigDecimal do checking type c...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/3208
[SQL][Minor] Let BigDecimal do checking type compatibility
Remove hardcoding max and min values for types. Let BigDecimal do checking type compatibility.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 more_numericLit
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3208.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 #3208
----
commit 1bdc69d03a3161b4f3c05873f80bc2e83e08adec
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2014-11-10T11:42:54Z
Let BigDecimal do checking type compatibility.
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20401148
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringOperations.scala ---
@@ -257,9 +257,16 @@ case class Substring(str: Expression, pos: Expression, len: Expression) extends
if ((string == null) || (po == null) || (ln == null)) {
null
} else {
- val start = po.asInstanceOf[Int]
- val length = ln.asInstanceOf[Int]
-
+ val start = po match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
+ }
+ val length = ln match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
+ }
--- End diff --
`start` and `length` are used as the parameters to call `slice`. Not sure if it is ok to change `slice` to use `Long`?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20392528
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala ---
@@ -339,18 +339,15 @@ class SqlParser extends AbstractSparkSQLParser {
| floatLit ^^ { f => Literal(f.toDouble) }
)
- private val longMax = BigDecimal(s"${Long.MaxValue}")
- private val longMin = BigDecimal(s"${Long.MinValue}")
- private val intMax = BigDecimal(s"${Int.MaxValue}")
- private val intMin = BigDecimal(s"${Int.MinValue}")
-
private def toNarrowestIntegerType(value: String) = {
val bigIntValue = BigDecimal(value)
bigIntValue match {
- case v if v < longMin || v > longMax => v
- case v if v < intMin || v > intMax => v.toLong
- case v => v.toInt
+ case v if bigIntValue.isValidByte => v.toByteExact
+ case v if bigIntValue.isValidShort => v.toShortExact
+ case v if bigIntValue.isValidInt => v.toIntExact
+ case v if bigIntValue.isValidLong => v.toLongExact
+ case v => v
--- End diff --
+1 this seems like a cleaner way to do 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/3208#issuecomment-64729341
Hi @marmbrus Is this ok to be merged?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20775962
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala ---
@@ -339,18 +339,15 @@ class SqlParser extends AbstractSparkSQLParser {
| floatLit ^^ { f => Literal(f.toDouble) }
)
- private val longMax = BigDecimal(s"${Long.MaxValue}")
- private val longMin = BigDecimal(s"${Long.MinValue}")
- private val intMax = BigDecimal(s"${Int.MaxValue}")
- private val intMin = BigDecimal(s"${Int.MinValue}")
-
private def toNarrowestIntegerType(value: String) = {
val bigIntValue = BigDecimal(value)
bigIntValue match {
- case v if v < longMin || v > longMax => v
- case v if v < intMin || v > intMax => v.toLong
- case v => v.toInt
+ case v if bigIntValue.isValidByte => v.toByteExact
+ case v if bigIntValue.isValidShort => v.toShortExact
+ case v if bigIntValue.isValidInt => v.toIntExact
+ case v if bigIntValue.isValidLong => v.toLongExact
+ case v => v
--- End diff --
Recently I have debugged few bugs in Presto database. So I tried to look at how Presto treat the integer literal. I found that it just uses Long to represent all number types. Although it does not meant all SQL systems doing this, I think that it can be a reference here.
So I will remove the byte and short types in this PR and let this PR can be merged then. Thanks.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20477025
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala ---
@@ -460,6 +460,20 @@ trait HiveTypeCoercion {
// Skip nodes who's children have not been resolved yet.
case e if !e.childrenResolved => e
+ case g @ GetItem(c, o @ IntegralType()) if o.dataType != IntegerType =>
+ GetItem(c, Cast(o, IntegerType))
--- End diff --
Indentation is off (2 spaces from `case`).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20477073
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala ---
@@ -460,6 +460,20 @@ trait HiveTypeCoercion {
// Skip nodes who's children have not been resolved yet.
case e if !e.childrenResolved => e
+ case g @ GetItem(c, o @ IntegralType()) if o.dataType != IntegerType =>
+ GetItem(c, Cast(o, IntegerType))
+
+ case s @ Substring(r, p @ IntegralType(), l @ IntegralType()) =>
+ var new_p = p
+ var new_l = l
+ if (p.dataType != IntegerType) {
+ new_p = Cast(p, IntegerType)
+ }
+ if (l.dataType != IntegerType) {
+ new_l = Cast(l, IntegerType)
+ }
+ Substring(r, new_p, new_l)
--- End diff --
It might be clearer / shorter to do this as two separate rules. I personally try to avoid `var` whenever possible.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20493243
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala ---
@@ -460,6 +460,20 @@ trait HiveTypeCoercion {
// Skip nodes who's children have not been resolved yet.
case e if !e.childrenResolved => e
+ case g @ GetItem(c, o @ IntegralType()) if o.dataType != IntegerType =>
+ GetItem(c, Cast(o, IntegerType))
+
+ case s @ Substring(r, p @ IntegralType(), l @ IntegralType()) =>
+ var new_p = p
+ var new_l = l
+ if (p.dataType != IntegerType) {
+ new_p = Cast(p, IntegerType)
+ }
+ if (l.dataType != IntegerType) {
+ new_l = Cast(l, IntegerType)
+ }
+ Substring(r, new_p, new_l)
--- End diff --
New commit makes it clearer and shorter.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20392753
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypes.scala ---
@@ -55,7 +55,11 @@ case class GetItem(child: Expression, ordinal: Expression) extends Expression {
// TODO: consider using Array[_] for ArrayType child to avoid
// boxing of primitives
val baseValue = value.asInstanceOf[Seq[_]]
- val o = key.asInstanceOf[Int]
+ val o = key match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
--- End diff --
I'm not sure that runtime type checking is really the best way to handle this. It adds a reflective cost to each invocation and it means when we add numeric types we need to revisit this code. Also, what about long?
Instead, if we want to support using bytes/shorts/etc as ordinals I think we should instead add a rule to analysis (probably in `FunctionArgumentConversion`) that adds a cast as needed.
Existing problem: `resolved` should not be true unless `ordinal` is of type `IntegerType`
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20392813
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringOperations.scala ---
@@ -257,9 +257,16 @@ case class Substring(str: Expression, pos: Expression, len: Expression) extends
if ((string == null) || (po == null) || (ln == null)) {
null
} else {
- val start = po.asInstanceOf[Int]
- val length = ln.asInstanceOf[Int]
-
+ val start = po match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
+ }
+ val length = ln match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
+ }
--- End diff --
Same concerns as above.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/3208#issuecomment-62676407
When parsing NumericLiteral, using more specified numeric types including Byte, Short that may improve memory efficiency slightly.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20688295
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala ---
@@ -339,18 +339,15 @@ class SqlParser extends AbstractSparkSQLParser {
| floatLit ^^ { f => Literal(f.toDouble) }
)
- private val longMax = BigDecimal(s"${Long.MaxValue}")
- private val longMin = BigDecimal(s"${Long.MinValue}")
- private val intMax = BigDecimal(s"${Int.MaxValue}")
- private val intMin = BigDecimal(s"${Int.MinValue}")
-
private def toNarrowestIntegerType(value: String) = {
val bigIntValue = BigDecimal(value)
bigIntValue match {
- case v if v < longMin || v > longMax => v
- case v if v < intMin || v > intMax => v.toLong
- case v => v.toInt
+ case v if bigIntValue.isValidByte => v.toByteExact
+ case v if bigIntValue.isValidShort => v.toShortExact
+ case v if bigIntValue.isValidInt => v.toIntExact
+ case v if bigIntValue.isValidLong => v.toLongExact
+ case v => v
--- End diff --
Okay, sorry, I realize I initially said this was a good idea. Thinking about it further though I'm not sure if this is actually something we want to do. The memory benefits of picking the smallest possible number representation don't really seem to outweigh the added complexity of having to deal with bytes everywhere all of a sudden.
Are there any other SQL systems that do this?
To be clear I am in favor of using BigDecimal's `isValidX` instead of our hand coded checking for int/long/bigdecimal
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3208
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/3208#issuecomment-63142516
Thanks for working on 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20493215
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala ---
@@ -460,6 +460,20 @@ trait HiveTypeCoercion {
// Skip nodes who's children have not been resolved yet.
case e if !e.childrenResolved => e
+ case g @ GetItem(c, o @ IntegralType()) if o.dataType != IntegerType =>
+ GetItem(c, Cast(o, IntegerType))
--- End diff --
Thanks. It is fixed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20392904
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala ---
@@ -47,6 +47,8 @@ object Literal {
object IntegerLiteral {
def unapply(a: Any): Option[Int] = a match {
case Literal(a: Int, IntegerType) => Some(a)
+ case Literal(a: Byte, ByteType) => Some(a.toInt)
+ case Literal(a: Short, ShortType) => Some(a.toInt)
--- End diff --
Hmm, it seems a little odd to be doing subtle type conversion here, especially since the name is `IntegerLiteral`. If it was `Integral`, maybe... I'm not super opposed, but is there a reason you want to make this change?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/3208#issuecomment-65136249
Thanks! Merged to master.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20401183
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala ---
@@ -47,6 +47,8 @@ object Literal {
object IntegerLiteral {
def unapply(a: Any): Option[Int] = a match {
case Literal(a: Int, IntegerType) => Some(a)
+ case Literal(a: Byte, ByteType) => Some(a.toInt)
+ case Literal(a: Short, ShortType) => Some(a.toInt)
--- End diff --
There are few places that use `IntegerLiteral` to extract numeric value from Literal. Because `NumericLiteral` could actually be parsed to `Byte`, `Short` now, `IntegerLiteral` also needs to add the support for them.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][Minor] Let BigDecimal do checking type c...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3208#issuecomment-62582301
Can one of the admins verify this patch?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20705901
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala ---
@@ -339,18 +339,15 @@ class SqlParser extends AbstractSparkSQLParser {
| floatLit ^^ { f => Literal(f.toDouble) }
)
- private val longMax = BigDecimal(s"${Long.MaxValue}")
- private val longMin = BigDecimal(s"${Long.MinValue}")
- private val intMax = BigDecimal(s"${Int.MaxValue}")
- private val intMin = BigDecimal(s"${Int.MinValue}")
-
private def toNarrowestIntegerType(value: String) = {
val bigIntValue = BigDecimal(value)
bigIntValue match {
- case v if v < longMin || v > longMax => v
- case v if v < intMin || v > intMax => v.toLong
- case v => v.toInt
+ case v if bigIntValue.isValidByte => v.toByteExact
+ case v if bigIntValue.isValidShort => v.toShortExact
+ case v if bigIntValue.isValidInt => v.toIntExact
+ case v if bigIntValue.isValidLong => v.toLongExact
+ case v => v
--- End diff --
I have no idea if any other SQL systems do like this. The only extra complexity it adds is the type casting in `GetItem` and `Substring` as I see. But I can not tell if the memory benefits is worth doing this. It may depend on how often the type casting is happened. If the time complexity issue is a big concern here, I can remove the byte and short types. Please let me know that. Thanks.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4358][SQL] Let BigDecimal do checking t...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/3208#discussion_r20401128
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypes.scala ---
@@ -55,7 +55,11 @@ case class GetItem(child: Expression, ordinal: Expression) extends Expression {
// TODO: consider using Array[_] for ArrayType child to avoid
// boxing of primitives
val baseValue = value.asInstanceOf[Seq[_]]
- val o = key.asInstanceOf[Int]
+ val o = key match {
+ case k if k.isInstanceOf[Byte] => k.asInstanceOf[Byte]
+ case k if k.isInstanceOf[Short] => k.asInstanceOf[Short]
+ case k => k.asInstanceOf[Int]
--- End diff --
The runtime type checking here is not good. Thanks for your suggestion, I add the casting for `GetItem` and `Substring` in `FunctionArgumentConversion`.
For the `resolved ` problem, since it is supported to use non `IntegerType` as `ordinal` (ref: test `apply schema` in `SQLQuerySuite`, to add this condition to `resolved` would case error. So I leave it as unmodified.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org