[SPARK-20098][PYSPARK] dataType's typeName fix by szalai1 · Pull Request #17435 · apache/spark

szalai1 · 2017-03-26T18:16:31Z

What changes were proposed in this pull request?

typeName classmethod has been fixed by using type -> typeName map.

How was this patch tested?

local build

HyukjinKwon · 2017-03-27T01:13:10Z

+ "ShortType": "short",
+ "ArrayType": "array",
+ "MapType": "map",
+ "StructField": "struct",


It seems this problem only applies to StructField. Could we just override typeName with simply throwing an exception? I think users are not supposed to call typeName against StructField but simpleString against the instance.
BTW, It apparently seems a bit odd that it extends DataType though.. I guess probably some tests are broken if we change the parent as it seems it is dependent on the parent assuming from #1598. So, I guess minimised fix would be just to override it.

cc @holdenk and @viirya WDYT?

Yeah, I don't think it is valid to call typeName against a StructField. Actually, StructField is not a data type, strictly speaking...
I don't know why StructField inherits DataType in pyspark. In scala, it is not.
Throwing an exception when calling typeName on StructField seems good enough, instead of a map like this.

@viirya The way I found this bug:
I wanted to figure out the schema of a dataset. I loaded it into a data frame and asked its schema. Then I called the typeName on each column. I do not know this is/was best way to do this, but I think it is valid to call typeName against a dataType to get its real type.

It is valid call for DataTypes but I don't think it is against StructField. If you look at the Scala-side codes, StructField is not a DataType (which is "The base type of all Spark SQL data types") but it seems the parent became DataType in Python for some reason in the PR I pointed out. In any way, It seems "A field inside a StructType".
If you want to know the schema, you could simply
>>>spark.range(1).schema[0].simpleString() 'id:bigint'>>>spark.range(1).schema.simpleString() 'struct<id:bigint>'>>> type(spark.range(1).schema[0]) <class'pyspark.sql.types.StructField'>>>>str(spark.range(1).schema[0]) 'StructField(id,LongType,false)'
Could you elaborate your use-case and why simpleString is not enough?

@szalai1 I think @HyukjinKwon 's code snippets should address your request. Doesn't it?

Btw, I don't think i.typeName() is a valid usage. We better let it throw an exception when calling typeName on StructField.
i.dataType.typeName() is more reasonable call to me.

@viirya You are right. I will use it that way. Thanks @HyukjinKwon !
But my bug report is still valid, I think. Can I override the typeName function?

Yup, I think we should still fix and overriding it is good enough.

HyukjinKwon · 2017-05-07T14:54:14Z

@szalai1, could you update this PR please? Otherwise, we should close this as a stale PR.

szalai1 · 2017-05-08T09:15:04Z

@HyukjinKwon sure, I will do it this week. I totally forgot this. Sorry.

HyukjinKwon · 2017-05-18T02:14:33Z

I think we need a test and @holdenk's review.

holdenk · 2017-05-20T16:40:19Z

Thanks for working on this. I feel like the error message could maybe be improved to suggest what the user should be doing? It would be nicer to eventually not have this depend on DataType since we don't have this in the Scala version as @HyukjinKwon pointed out, but I think this could be a good improvement for now.

szalai1 · 2017-05-20T17:47:47Z

@holdenk I am happy to contribute to this project. I changed the error message and added a test case.

holdenk · 2017-06-15T09:40:17Z

Sorry for my delay finishing the review. This looks good pending Jenkins and if it passes I'll try and merge it when I get home. (so Jenkins looks good to test. Jenkins test this please. ) :)

SparkQA · 2017-06-15T09:43:59Z

Test build #78097 has finished for PR 17435 at commit 6bf11f0.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2017-06-17T01:09:07Z

Hey @szalai1 if you've got time to address the style issues its looking good otherwise :)

szalai1 · 2017-06-25T21:04:59Z

@holdenk Thanks for reviewing it. I fixed the style issues.

ueshin · 2017-07-28T03:10:16Z

ok to test

SparkQA · 2017-07-28T03:30:00Z

Test build #80013 has finished for PR 17435 at commit 8872e19.

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

ueshin · 2017-07-28T03:34:28Z

@szalai1 Could you fix tests if you're still working on this please?

szalai1 · 2017-08-10T07:31:22Z

@ueshin Sure, I'll do it next week.

holdenk · 2017-09-06T20:13:18Z

Gentle ping (going through old PySpark PRs) :)

SparkQA · 2017-09-07T08:18:44Z

Test build #81511 has finished for PR 17435 at commit a18436d.

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

szalai1 · 2017-09-07T11:02:22Z

@holdenk thanks for reminder. I fixed the problem.

HyukjinKwon · 2017-09-08T00:04:12Z

 return self.dataType.fromInternal(obj)

+ def typeName(self):
+ raise TypeError(


Could we do like ...
raiseTypeError( "...""...")
if it doesn't bother you much?

HyukjinKwon · 2017-09-08T00:16:30Z

+ def typeName(self):
+ raise TypeError(
+ "StructField does not have typename. \
+ You can use self.dataType.simpleString() instead.")


I'd remove self here and just say something like use typeName() on its type explicitly ....

you mean simpleString() and not typeName(). right ?

I think it should be typeName on its datatype because typeName was called.

HyukjinKwon · 2017-09-08T00:17:37Z


+ def typeName(self):
+ raise TypeError(
+ "StructField does not have typename. \


Little nit: looks a typo, typename -> typeName.

SparkQA · 2017-09-08T11:56:20Z

Test build #81556 has finished for PR 17435 at commit a451901.

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

HyukjinKwon · 2017-09-08T11:59:24Z


+ def typeName(self):
+ raise TypeError(
+ "StructField does not have typeName."


I am sorry. Could you add a space at the end ..eName." -> ...eName. "?

SparkQA · 2017-09-08T13:08:04Z

Test build #81559 has finished for PR 17435 at commit 98bac12.

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

holdenk · 2017-09-08T19:51:44Z

@szalai1 I think @HyukjinKwon wanted the space on the previous line/sentence.

SparkQA · 2017-09-09T08:17:53Z

Test build #81584 has finished for PR 17435 at commit 28f142c.

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

HyukjinKwon · 2017-09-09T08:26:52Z

LGTM

## What changes were proposed in this pull request? `typeName` classmethod has been fixed by using type -> typeName map. ## How was this patch tested? local build Author: Peter Szalai <szalaipeti.vagyok@gmail.com> Closes#17435 from szalai1/datatype-gettype-fix. (cherry picked from commit 520d92a) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

HyukjinKwon · 2017-09-10T08:52:25Z

Merged to master and branch-2.2.

## What changes were proposed in this pull request? `typeName` classmethod has been fixed by using type -> typeName map. ## How was this patch tested? local build Author: Peter Szalai <szalaipeti.vagyok@gmail.com> Closesapache#17435 from szalai1/datatype-gettype-fix. (cherry picked from commit 520d92a) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

szalai1 added 2 commits March 26, 2017 19:51

getType fix
00994e9

type
933f3cb

HyukjinKwon reviewed Mar 27, 2017
View reviewed changes

StructField's typeName raise TypeError
0a18192

srowen mentioned this pull request May 17, 2017
[INFRA] Close stale PRs #18017
Closed

add test
6bf11f0

fixing pep8 style issues
8872e19

test failure fixed
a18436d

HyukjinKwon reviewed Sep 8, 2017
View reviewed changes

improving error message
a451901

HyukjinKwon reviewed Sep 8, 2017
View reviewed changes

adding space
98bac12

space at thr right place
28f142c

asfgit closed this in 520d92aSep 10, 2017

Uh oh!

Conversation

szalai1 commented Mar 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwonMar 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonMar 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented May 7, 2017

Uh oh!

szalai1 commented May 8, 2017

Uh oh!

HyukjinKwon commented May 18, 2017

Uh oh!

holdenk commented May 20, 2017

Uh oh!

szalai1 commented May 20, 2017

Uh oh!

holdenk commented Jun 15, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

holdenk commented Jun 17, 2017

Uh oh!

szalai1 commented Jun 25, 2017

Uh oh!

ueshin commented Jul 28, 2017

Uh oh!

SparkQA commented Jul 28, 2017

Uh oh!

ueshin commented Jul 28, 2017

Uh oh!

szalai1 commented Aug 10, 2017

Uh oh!

holdenk commented Sep 6, 2017

Uh oh!

SparkQA commented Sep 7, 2017

Uh oh!

szalai1 commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szalai1Sep 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 8, 2017

szalai1 commented Mar 26, 2017•
edited
Loading

HyukjinKwonMar 27, 2017•
edited
Loading

HyukjinKwonMar 27, 2017•
edited
Loading

szalai1Sep 8, 2017•
edited
Loading