[SPARK-3363][SQL] Type Coercion should support every type to have null value #2246

adrian-wang · 2014-09-03T05:17:57Z

Type Coercion should support every type to have null value

SparkQA · 2014-09-03T05:24:33Z

QA tests have started for PR 2246 at commit c619f0a.

This patch merges cleanly.

SparkQA · 2014-09-03T07:11:02Z

QA tests have finished for PR 2246 at commit c619f0a.

This patch fails unit tests.
This patch merges cleanly.
This patch adds no public classes.

adrian-wang · 2014-09-03T07:33:35Z

JsonRDD also called HiveTypeCoercion.allPromotion, but with a method owned by itself, which leads to the failure. I have fixed that also.

SparkQA · 2014-09-03T07:39:57Z

QA tests have started for PR 2246 at commit 46daa90.

This patch merges cleanly.

SparkQA · 2014-09-03T09:27:07Z

QA tests have finished for PR 2246 at commit 46daa90.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

marmbrus · 2014-09-04T01:56:50Z

sql/core/src/main/scala/org/apache/spark/sql/json/JsonRDD.scala

-    // If found return the widest common type, otherwise None
-    val returnType = applicableConversion.map(_.filter(t => t == t1 || t == t2).last)
+      // If found return the widest common type, otherwise None
+      applicableConversion.map(_.filter(t => t == t1 || t == t2).last)


Is there a reason we can't just call findTightestCommonType here?

Yes, in complex type they are different. For example what JSON cares are if the key strings are the same(MapType), which is quite different from here.

Well, yes after this the code is different, but up here this looks identical. Could this just be val returnType = findTightestCommonType(t1, t2)? (Sorry if I'm missing something obvious)

But I think in the future we should also support complex type promotion in findTightestCommonType(t1, t2). Anyway we can extract the current findTightestCommonType(t1, t2) as a util function. What do you prefer?

Yeah, I don't see any reason to over plan for possible extension in the future. Since this code is the same in both places I think it'd be better to just call findTightestCommonType instead of duplicating it. If we decide to add more rules in the future we can add a new function that calls this simpler version.

adrian-wang · 2014-09-04T05:47:34Z

I made a mistake here, This part cannot use findTightestCommonType(t1, t2), because we should not return the value only if the 2 types are the same. That is, for two Struct(field) type, we need to compare the field name to decide.

SparkQA · 2014-09-04T06:14:13Z

QA tests have started for PR 2246 at commit ef6f986.

This patch merges cleanly.

adrian-wang · 2014-09-04T06:14:26Z

I have changed the code, now compatibleType and findTightestCommonType looks different.

SparkQA · 2014-09-04T07:51:51Z

QA tests have finished for PR 2246 at commit ef6f986.

This patch fails unit tests.
This patch merges cleanly.
This patch adds no public classes.

adrian-wang · 2014-09-04T07:53:25Z

I hate graphx :(

rxin · 2014-09-04T07:54:19Z

Don't hate it :)

rxin · 2014-09-04T07:54:24Z

Jenkins, retest this please.

SparkQA · 2014-09-04T07:59:15Z

QA tests have started for PR 2246 at commit ef6f986.

This patch merges cleanly.

SparkQA · 2014-09-04T09:53:25Z

QA tests have finished for PR 2246 at commit ef6f986.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

marmbrus · 2014-09-05T01:44:37Z

I don't think the concern about using findTightestCommonType with struct types is valid. If the fields match up exactly we will return the type, otherwise we will fall back on the advanced resolution logic.

Here is a PR (against your PR) that implements this and seems to pass all our tests: adrian-wang#2

adrian-wang · 2014-09-05T05:16:59Z

Hmm... you are right, I misunderstood some code in compatibleType there. Thank you!

reduce code duplication

SparkQA · 2014-09-05T23:42:12Z

QA tests have started for PR 2246 at commit c6241de.

This patch merges cleanly.

SparkQA · 2014-09-06T00:49:19Z

QA tests have finished for PR 2246 at commit c6241de.

This patch fails unit tests.
This patch merges cleanly.
This patch adds no public classes.

marmbrus · 2014-09-10T01:42:18Z

Jenkins, test this please

SparkQA · 2014-09-10T01:51:14Z

QA tests have started for PR 2246 at commit c6241de.

This patch merges cleanly.

SparkQA · 2014-09-10T03:30:26Z

QA tests have finished for PR 2246 at commit c6241de.

This patch fails unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class LastFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class Abs(child: Expression) extends UnaryExpression

adrian-wang · 2014-09-10T10:41:47Z

The fail is in BroadcastSuite, has nothing to do with this PR...

marmbrus · 2014-09-10T17:39:46Z

@JoshRosen, another flakey one in Core

marmbrus · 2014-09-10T17:49:05Z

Thanks! Merged to master.

Type Coercion should support every type to have null value

c619f0a

adrian-wang changed the title ~~Type Coercion should support every type to have null value~~ [SPARK-3363][SQL] Type Coercion should support every type to have null value Sep 3, 2014

marmbrus reviewed Sep 4, 2014
View reviewed changes

adrian-wang force-pushed the spark3363-0 branch from 01e7a7a to 46daa90 Compare September 4, 2014 05:43

make double boolean miss in jsonRDD compatibleType

ef6f986

adrian-wang force-pushed the spark3363-0 branch from 46daa90 to ef6f986 Compare September 4, 2014 06:09

reduce code duplication

832e640

adrian-wang added 2 commits September 5, 2014 13:18

Merge pull request #2 from marmbrus/pr/2246

595b417

reduce code duplication

minor code clean

c6241de

asfgit closed this in f0c87dc Sep 10, 2014

adrian-wang deleted the spark3363-0 branch September 24, 2014 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-3363][SQL] Type Coercion should support every type to have null value #2246

[SPARK-3363][SQL] Type Coercion should support every type to have null value #2246

adrian-wang commented Sep 3, 2014

SparkQA commented Sep 3, 2014

SparkQA commented Sep 3, 2014

adrian-wang commented Sep 3, 2014

SparkQA commented Sep 3, 2014

SparkQA commented Sep 3, 2014

marmbrus Sep 4, 2014

adrian-wang Sep 4, 2014

marmbrus Sep 4, 2014

adrian-wang Sep 4, 2014

marmbrus Sep 4, 2014

adrian-wang commented Sep 4, 2014

SparkQA commented Sep 4, 2014

adrian-wang commented Sep 4, 2014

SparkQA commented Sep 4, 2014

adrian-wang commented Sep 4, 2014

rxin commented Sep 4, 2014

rxin commented Sep 4, 2014

SparkQA commented Sep 4, 2014

SparkQA commented Sep 4, 2014

marmbrus commented Sep 5, 2014

adrian-wang commented Sep 5, 2014

SparkQA commented Sep 5, 2014

SparkQA commented Sep 6, 2014

marmbrus commented Sep 10, 2014

SparkQA commented Sep 10, 2014

SparkQA commented Sep 10, 2014

adrian-wang commented Sep 10, 2014

marmbrus commented Sep 10, 2014

marmbrus commented Sep 10, 2014

[SPARK-3363][SQL] Type Coercion should support every type to have null value #2246

[SPARK-3363][SQL] Type Coercion should support every type to have null value #2246

Conversation

adrian-wang commented Sep 3, 2014

SparkQA commented Sep 3, 2014

SparkQA commented Sep 3, 2014

adrian-wang commented Sep 3, 2014

SparkQA commented Sep 3, 2014

SparkQA commented Sep 3, 2014

marmbrus Sep 4, 2014

Choose a reason for hiding this comment

adrian-wang Sep 4, 2014

Choose a reason for hiding this comment

marmbrus Sep 4, 2014

Choose a reason for hiding this comment

adrian-wang Sep 4, 2014

Choose a reason for hiding this comment

marmbrus Sep 4, 2014

Choose a reason for hiding this comment

adrian-wang commented Sep 4, 2014

SparkQA commented Sep 4, 2014

adrian-wang commented Sep 4, 2014

SparkQA commented Sep 4, 2014

adrian-wang commented Sep 4, 2014

rxin commented Sep 4, 2014

rxin commented Sep 4, 2014

SparkQA commented Sep 4, 2014

SparkQA commented Sep 4, 2014

marmbrus commented Sep 5, 2014

adrian-wang commented Sep 5, 2014

SparkQA commented Sep 5, 2014

SparkQA commented Sep 6, 2014

marmbrus commented Sep 10, 2014

SparkQA commented Sep 10, 2014

SparkQA commented Sep 10, 2014

adrian-wang commented Sep 10, 2014

marmbrus commented Sep 10, 2014

marmbrus commented Sep 10, 2014