You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by dilipbiswal <gi...@git.apache.org> on 2018/09/25 08:58:50 UTC
[GitHub] spark pull request #22544: [SPARK-25522] Improve type promotion for input ar...
GitHub user dilipbiswal opened a pull request:
https://github.com/apache/spark/pull/22544
[SPARK-25522] Improve type promotion for input arguments of elementAt function
## What changes were proposed in this pull request?
In ElementAt, when first argument is MapType, we should coerce the key type and the second argument based on findTightestCommonType. This is not happening currently.
Also, when the first argument is ArrayType, the second argument should be an integer type or a smaller integral type that can be safely casted to an integer type. Currently we may do an unsafe cast.
```SQL
spark-sql> select element_at(array(1,2), 1.24);
1
```
```SQL
spark-sql> select element_at(map(1,"one", 2, "two"), 2.2);
two
```
This PR also supports implicit cast between two MapTypes. I have followed similar logic that exists today to do implicit casts between two array types.
## How was this patch tested?
Added new tests in DataFrameFunctionSuite, TypeCoercionSuite.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dilipbiswal/spark SPARK-25522
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22544.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 #22544
----
commit 4d32f2ce2ff4d5e13c693053041d3111526662cb
Author: Dilip Biswal <db...@...>
Date: 2018-09-16T02:36:18Z
[SPARK-25522] Improve type promotion for input arguments of elementAt function.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96603 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96603/testReport)** for PR 22544 at commit [`48eb0ff`](https://github.com/apache/spark/commit/48eb0ff996f48cae49a0ece90160ac5a1251b4b8).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96660 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96660/testReport)** for PR 22544 at commit [`69c78d7`](https://github.com/apache/spark/commit/69c78d7655d0f730f0a72cb8fc98b1491de53da1).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220452974
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,38 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
--- End diff --
Could you add a note that we can't use force-nullable key types.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96603/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220532183
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -974,6 +974,33 @@ object TypeCoercion {
if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true))
+ if !Cast.forceNullable(fromKeyType, toKeyType) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, true)
+ } else {
+ null
+ }
+
+ case (MapType(fromKeyType, fromValueType, true), MapType(toKeyType, toValueType, false)) =>
+ null
+
+ case (MapType(fromKeyType, fromValueType, false), MapType(toKeyType, toValueType, false))
+ if (!(Cast.forceNullable(fromKeyType, toKeyType) ||
+ Cast.forceNullable(fromValueType, toValueType))) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, false)
+ } else {
+ null
+ }
+
--- End diff --
then it's just one case
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220445443
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,38 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
--- End diff --
nit: revert this change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220456613
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,38 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true))
+ if !Cast.forceNullable(fromKeyType, toKeyType) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null
+ && (!newKeyType.sameType(fromKeyType) || !newValueType.sameType(fromValueType))) {
--- End diff --
@ueshin U r right. Let me take it out and run the tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220460094
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala ---
@@ -257,12 +257,48 @@ class TypeCoercionSuite extends AnalysisTest {
shouldNotCast(checkedType, IntegralType)
}
- test("implicit type cast - MapType(StringType, StringType)") {
- val checkedType = MapType(StringType, StringType)
- checkTypeCasting(checkedType, castableTypes = Seq(checkedType))
- shouldNotCast(checkedType, DecimalType)
- shouldNotCast(checkedType, NumericType)
- shouldNotCast(checkedType, IntegralType)
+ test("implicit type cast between two Map types") {
+ val sourceType = MapType(IntegerType, IntegerType, true)
+ val castableTypes = numericTypes ++ Seq(StringType).filter(!Cast.forceNullable(IntegerType, _))
+ val targetTypes = numericTypes.filter(!Cast.forceNullable(IntegerType, _)).map { t =>
+ MapType(t, sourceType.valueType, valueContainsNull = true)
+ }
+ val nonCastableTargetTypes = allTypes.filterNot(castableTypes.contains(_)).map {t =>
+ MapType(t, sourceType.valueType, valueContainsNull = true)
+ }
+
+ // Tests that its possible to setup implicit casts between two map types when
+ // source map's key type is integer and the target map's key type are either Byte, Short,
+ // Long, Double, Float, Decimal(38, 18) or String.
+ targetTypes.foreach { targetType =>
+ shouldCast(sourceType, targetType, targetType)
+ }
+
+ // Tests that its not possible to setup implicit casts between two map types when
+ // source map's key type is integer and the target map's key type are either Binary,
+ // Boolean, Date, Timestamp, Array, Struct, CaleandarIntervalType or NullType
+ nonCastableTargetTypes.foreach { targetType =>
+ shouldNotCast(sourceType, targetType)
+ }
+
+ // Tests that its not possible to cast from nullable map type to not nullable map type.
+ val targetNotNullableTypes = allTypes.filterNot(_ == IntegerType).map { t =>
+ MapType(t, sourceType.valueType, valueContainsNull = false)
+ }
+ val sourceMapExprWithValueNull =
+ CreateMap(Seq(Literal.default(sourceType.keyType),
+ Literal.create(null, sourceType.valueType)))
+ targetNotNullableTypes.foreach { targetType =>
+ val x = 10
+ val castDefault =
+ TypeCoercion.ImplicitTypeCasts.implicitCast(sourceMapExprWithValueNull, targetType)
+ assert(castDefault.isEmpty,
+ s"Should not be able to cast $sourceType to $sourceType, but got $castDefault")
+ }
+
+ shouldNotCast(MapType(DoubleType, DoubleType, valueContainsNull = false),
+ CalendarIntervalType)
--- End diff --
why there is a test for `CalendarIntervalType`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22544
After thinking more, I'm going it to merge it to 2.4 directly, because:
1. `ImplicitTypeCasts` handles array type but not map type, which looks a bug to me
2. I checked all the implementations of `ImplicitCastInputTypes`, `ElementAt` is the only one that sets a concrete map type as the input type. So this change only affects `ElementAt`
3. the test cases added in this PR iterate all the cases and have a very good converage.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220533930
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, hasNull), e2: IntegralType) if (e2 != LongType) =>
--- End diff --
nit:
```
case (arr: ArrayType, e2: IntegralType) if ... =>
Seq(arr, IntegerType)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96644/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220531899
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -974,6 +974,33 @@ object TypeCoercion {
if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true))
+ if !Cast.forceNullable(fromKeyType, toKeyType) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, true)
+ } else {
+ null
+ }
+
+ case (MapType(fromKeyType, fromValueType, true), MapType(toKeyType, toValueType, false)) =>
+ null
+
+ case (MapType(fromKeyType, fromValueType, false), MapType(toKeyType, toValueType, false))
+ if (!(Cast.forceNullable(fromKeyType, toKeyType) ||
+ Cast.forceNullable(fromValueType, toValueType))) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, false)
+ } else {
+ null
+ }
+
--- End diff --
nit:
```
case (MapType(fromKeyType, fromValueType, fromNullable), MapType(toKeyType, toValueType, toNullable))
if !Cast.forceNullable(fromKeyType, toKeyType) && Cast.resolvableNullability(fromNullable, toNullable)
....
if (Cast.forceNullable(fromValueType, toValueType) $$ !toNullable) {
null
} else {
MapType(newKeyType, newValueType, toNullable)
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96620 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96620/testReport)** for PR 22544 at commit [`2f224f2`](https://github.com/apache/spark/commit/2f224f2710c6adcdf88725a1aef02a2d96437d49).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96660/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96611/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96660 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96660/testReport)** for PR 22544 at commit [`69c78d7`](https://github.com/apache/spark/commit/69c78d7655d0f730f0a72cb8fc98b1491de53da1).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/22544
@dilipbiswal The two examples shown in the PR description are allowed in current and this proposes to disallow them? I'm a bit confused by the description.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220459877
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala ---
@@ -257,12 +257,48 @@ class TypeCoercionSuite extends AnalysisTest {
shouldNotCast(checkedType, IntegralType)
}
- test("implicit type cast - MapType(StringType, StringType)") {
- val checkedType = MapType(StringType, StringType)
- checkTypeCasting(checkedType, castableTypes = Seq(checkedType))
- shouldNotCast(checkedType, DecimalType)
- shouldNotCast(checkedType, NumericType)
- shouldNotCast(checkedType, IntegralType)
+ test("implicit type cast between two Map types") {
+ val sourceType = MapType(IntegerType, IntegerType, true)
+ val castableTypes = numericTypes ++ Seq(StringType).filter(!Cast.forceNullable(IntegerType, _))
+ val targetTypes = numericTypes.filter(!Cast.forceNullable(IntegerType, _)).map { t =>
+ MapType(t, sourceType.valueType, valueContainsNull = true)
+ }
+ val nonCastableTargetTypes = allTypes.filterNot(castableTypes.contains(_)).map {t =>
+ MapType(t, sourceType.valueType, valueContainsNull = true)
+ }
+
+ // Tests that its possible to setup implicit casts between two map types when
+ // source map's key type is integer and the target map's key type are either Byte, Short,
+ // Long, Double, Float, Decimal(38, 18) or String.
+ targetTypes.foreach { targetType =>
+ shouldCast(sourceType, targetType, targetType)
+ }
+
+ // Tests that its not possible to setup implicit casts between two map types when
+ // source map's key type is integer and the target map's key type are either Binary,
+ // Boolean, Date, Timestamp, Array, Struct, CaleandarIntervalType or NullType
+ nonCastableTargetTypes.foreach { targetType =>
+ shouldNotCast(sourceType, targetType)
+ }
+
+ // Tests that its not possible to cast from nullable map type to not nullable map type.
+ val targetNotNullableTypes = allTypes.filterNot(_ == IntegerType).map { t =>
+ MapType(t, sourceType.valueType, valueContainsNull = false)
+ }
+ val sourceMapExprWithValueNull =
+ CreateMap(Seq(Literal.default(sourceType.keyType),
+ Literal.create(null, sourceType.valueType)))
+ targetNotNullableTypes.foreach { targetType =>
+ val x = 10
+ val castDefault =
+ TypeCoercion.ImplicitTypeCasts.implicitCast(sourceMapExprWithValueNull, targetType)
+ assert(castDefault.isEmpty,
+ s"Should not be able to cast $sourceType to $sourceType, but got $castDefault")
--- End diff --
`to $sourceType` -> `to $targetType`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3511/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220452656
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,38 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true))
+ if !Cast.forceNullable(fromKeyType, toKeyType) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null
+ && (!newKeyType.sameType(fromKeyType) || !newValueType.sameType(fromValueType))) {
--- End diff --
Do we need this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96620/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96620 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96620/testReport)** for PR 22544 at commit [`2f224f2`](https://github.com/apache/spark/commit/2f224f2710c6adcdf88725a1aef02a2d96437d49).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3467/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220140252
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true)) =>
+ val newFromType = implicitCast(fromKeyType, toKeyType).orNull
--- End diff --
I think this should be named as `newKeyType`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96644 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96644/testReport)** for PR 22544 at commit [`9b12969`](https://github.com/apache/spark/commit/9b129698d95c14151b560f16942bda1f06cb4749).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22544
@cloud-fan @ueshin @viirya Thank you very much.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220241139
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, hasNull), e2: IntegralType) if (e2 != LongType) =>
+ Seq(ArrayType(e1, hasNull), IntegerType)
+ case (MapType(keyType, valueType, hasNull), e2) =>
+ TypeCoercion.findTightestCommonType(keyType, e2) match {
+ case Some(dt) => Seq(MapType(dt, valueType, hasNull), dt)
+ case _ => Seq.empty
--- End diff --
Thanks @dilipbiswal for clarifying it. Can you update the PR description with such example, I feel it is more clear.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/22544
retest this please.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3481/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96544 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96544/testReport)** for PR 22544 at commit [`4d32f2c`](https://github.com/apache/spark/commit/4d32f2ce2ff4d5e13c693053041d3111526662cb).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96644 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96644/testReport)** for PR 22544 at commit [`9b12969`](https://github.com/apache/spark/commit/9b129698d95c14151b560f16942bda1f06cb4749).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96603 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96603/testReport)** for PR 22544 at commit [`48eb0ff`](https://github.com/apache/spark/commit/48eb0ff996f48cae49a0ece90160ac5a1251b4b8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22544
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the issue:
https://github.com/apache/spark/pull/22544
cc @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220765052
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -974,6 +974,25 @@ object TypeCoercion {
if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above. Make sure that both key and values
+ // can not by null after the implicit cast operation by calling forceNullable
--- End diff --
nit: I guess this is `can not be null`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96544 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96544/testReport)** for PR 22544 at commit [`4d32f2c`](https://github.com/apache/spark/commit/4d32f2ce2ff4d5e13c693053041d3111526662cb).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96544/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220250338
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, hasNull), e2: IntegralType) if (e2 != LongType) =>
+ Seq(ArrayType(e1, hasNull), IntegerType)
+ case (MapType(keyType, valueType, hasNull), e2) =>
+ TypeCoercion.findTightestCommonType(keyType, e2) match {
+ case Some(dt) => Seq(MapType(dt, valueType, hasNull), dt)
+ case _ => Seq.empty
--- End diff --
@viirya Thanks .. i have updated the description. Please let me know if it looks okay to you or anything is amiss.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96611 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96611/testReport)** for PR 22544 at commit [`48eb0ff`](https://github.com/apache/spark/commit/48eb0ff996f48cae49a0ece90160ac5a1251b4b8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220766294
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -974,6 +974,25 @@ object TypeCoercion {
if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above. Make sure that both key and values
+ // can not by null after the implicit cast operation by calling forceNullable
--- End diff --
@viirya Thank you.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3430/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3474/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220772034
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (arr: ArrayType, e2: IntegralType) if (e2 != LongType) =>
+ Seq(arr, IntegerType)
+ case (MapType(keyType, valueType, hasNull), e2) =>
+ TypeCoercion.findTightestCommonType(keyType, e2) match {
+ case Some(dt) => Seq(MapType(dt, valueType, hasNull), dt)
+ case _ => Seq.empty
+ }
+ case (l, r) => Seq.empty
+
+ }
}
override def checkInputDataTypes(): TypeCheckResult = {
- super.checkInputDataTypes() match {
- case f: TypeCheckResult.TypeCheckFailure => f
- case TypeCheckResult.TypeCheckSuccess if left.dataType.isInstanceOf[MapType] =>
- TypeUtils.checkForOrderingExpr(mapKeyType, s"function $prettyName")
- case TypeCheckResult.TypeCheckSuccess => TypeCheckResult.TypeCheckSuccess
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, _), e2) if e2 != IntegerType =>
--- End diff --
nit: `e1` is not used, this can be `case (_: ArrayType, e2) if ...`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220193922
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, hasNull), e2: IntegralType) if (e2 != LongType) =>
+ Seq(ArrayType(e1, hasNull), IntegerType)
+ case (MapType(keyType, valueType, hasNull), e2) =>
+ TypeCoercion.findTightestCommonType(keyType, e2) match {
+ case Some(dt) => Seq(MapType(dt, valueType, hasNull), dt)
+ case _ => Seq.empty
--- End diff --
Previously it will do implicitly conversion on the right expr to map's key type. Map type is not converted at all.
This change finds the common type between right expr and map's key type and will do conversion on map and right expr.
Can you provide an example showing why this is needed?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220232666
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala ---
@@ -2140,21 +2140,34 @@ case class ElementAt(left: Expression, right: Expression) extends GetMapValueUti
}
override def inputTypes: Seq[AbstractDataType] = {
- Seq(TypeCollection(ArrayType, MapType),
- left.dataType match {
- case _: ArrayType => IntegerType
- case _: MapType => mapKeyType
- case _ => AnyDataType // no match for a wrong 'left' expression type
- }
- )
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, hasNull), e2: IntegralType) if (e2 != LongType) =>
+ Seq(ArrayType(e1, hasNull), IntegerType)
+ case (MapType(keyType, valueType, hasNull), e2) =>
+ TypeCoercion.findTightestCommonType(keyType, e2) match {
+ case Some(dt) => Seq(MapType(dt, valueType, hasNull), dt)
+ case _ => Seq.empty
--- End diff --
@viirya
```select element_at(map(1,"one", 2, "two"), 2.2D);```
In this case, there is no key with value 2.2 in the map, right ? But we return "two" as the answer. If we simply cast the right side to the key type of the map, then we will cast 2.2 down to 2 and find "two". But by finding a common type, we will compare both sides as double and correctly return "null" as the answer.
Here is presto's output :
```SQL
presto:default> select element_at(MAP(array[1], array[1]), 1.1);
_col0
-------
NULL
(1 row)
Query 20180921_051929_00018_iif98, FINISHED, 1 node
Splits: 17 total, 17 done (100.00%)
0:00 [0 rows, 0B] [0 rows/s, 0B/s]
presto:default> select element_at(MAP(array[1], array[1]), 1.0);
_col0
-------
1
(1 row)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220250631
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
--- End diff --
@viirya actually i followed the arraytype example. Actually it has decent coverage, i think. If you can think of a case thats missing , i will add. I will also go through once again myself.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220140660
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true)) =>
+ val newFromType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newFromType != null && newValueType != null
+ && (!newFromType.sameType(fromKeyType) || !newValueType.sameType(fromValueType))) {
+ MapType(newFromType, newValueType, true)
+ } else {
+ null
+ }
+
+ case (MapType(fromKeyType, fromValueType, true), MapType(toKeyType, toValueType, false)) =>
+ null
+
+ case (MapType(fromKeyType, fromValueType, false), MapType(toKeyType, toValueType, false))
+ if !Cast.forceNullable(fromValueType, toValueType) =>
+ val newFromType = implicitCast(fromKeyType, toKeyType).orNull
--- End diff --
ditto, `newKeyType`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220253485
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true)) =>
+ val newFromType = implicitCast(fromKeyType, toKeyType).orNull
--- End diff --
@viirya Will change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3498/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22544
@cloud-fan Sounds good Wenchen. Thanks for the detailed analysis.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22544
@cloud-fan Are we targeting this for 2.4 ? If so, i was wondering if we should make a restricted fix that affects only this function by adding a case in `FunctionArgumentConversion` for 2.4 while keeping this fix for master. Pl. let me know what you think.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220244473
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
--- End diff --
Do we have enough test for this new implicit cast rules?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22544
I'm merging it to master, please send another PR to 2.4, without touching the `findTightestCommonType`. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220147139
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -971,9 +971,36 @@ object TypeCoercion {
case (ArrayType(fromType, true), ArrayType(toType: DataType, false)) => null
case (ArrayType(fromType, false), ArrayType(toType: DataType, false))
- if !Cast.forceNullable(fromType, toType) =>
+ if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true)) =>
+ val newFromType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newFromType != null && newValueType != null
--- End diff --
Don't we need to verify `!Cast.forceNullable(fromKeyType, toKeyType)`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22544
**[Test build #96611 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96611/testReport)** for PR 22544 at commit [`48eb0ff`](https://github.com/apache/spark/commit/48eb0ff996f48cae49a0ece90160ac5a1251b4b8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22544: [SPARK-25522][SQL] Improve type promotion for inp...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on a diff in the pull request:
https://github.com/apache/spark/pull/22544#discussion_r220746708
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala ---
@@ -974,6 +974,33 @@ object TypeCoercion {
if !Cast.forceNullable(fromType, toType) =>
implicitCast(fromType, toType).map(ArrayType(_, false)).orNull
+ // Implicit cast between Map types.
+ // Follows the same semantics of implicit casting between two array types.
+ // Refer to documentation above.
+ case (MapType(fromKeyType, fromValueType, fn), MapType(toKeyType, toValueType, true))
+ if !Cast.forceNullable(fromKeyType, toKeyType) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, true)
+ } else {
+ null
+ }
+
+ case (MapType(fromKeyType, fromValueType, true), MapType(toKeyType, toValueType, false)) =>
+ null
+
+ case (MapType(fromKeyType, fromValueType, false), MapType(toKeyType, toValueType, false))
+ if (!(Cast.forceNullable(fromKeyType, toKeyType) ||
+ Cast.forceNullable(fromValueType, toValueType))) =>
+ val newKeyType = implicitCast(fromKeyType, toKeyType).orNull
+ val newValueType = implicitCast(fromValueType, toValueType).orNull
+ if (newKeyType != null && newValueType != null) {
+ MapType(newKeyType, newValueType, false)
+ } else {
+ null
+ }
+
--- End diff --
@cloud-fan Done. Please check when you get a chance.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22544: [SPARK-25522][SQL] Improve type promotion for input argu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22544
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org