Skip to content

[SPARK-21712] [PySpark] Clarify type error for Column.substr() #18926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nchammas wants to merge 4 commits into from
Closed

[SPARK-21712] [PySpark] Clarify type error for Column.substr() #18926

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
753dbe1
clarify type error for Column.substr()
nchammas Aug 11, 2017
ff9b07c
long is not supported
nchammas Aug 14, 2017
fc1d84f
add type tests for substr
nchammas Aug 14, 2017
a7fea20
check substr type error message
nchammas Aug 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion python/pyspark/sql/column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,13 @@ def substr(self, startPos, length):
[Row(col=u'Ali'), Row(col=u'Bob')]
"""
if type(startPos) != type(length):
raise TypeError("Can not mix the type")
raise TypeError(
"startPos and length must be the same type. "
"Got {startPos_t} and {length_t}, respectively."

@gatorsmile gatorsmile Aug 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> startPos: {startPos_t}; length: {length_t}.

BTW, why we do the type checking here, instead of doing it in the actual Scala impl of substr?

In addition, we do not support the mixed cases? For example, startPos is int, length is long.

@HyukjinKwon HyukjinKwon Aug 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why we do the type checking here, instead of doing it in the actual Scala impl of substr?

Do you mean exposing Java types in the error message is better or suggesting method signature change in Scala impl of substr with the check logic?

In addition, we do not support the mixed cases? For example, startPos is int, length is long.

In Python, I guess it makes sense calling int in general. long and int are unified in Python 3 and this PR looks targeting only the exception message fix.

@gatorsmile gatorsmile Aug 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PySpark always needs to check the types, are we doing the same things in all the other function calls?

In addition, why not directly checking

if isinstance(length, (int, long)):

@HyukjinKwon HyukjinKwon Aug 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to check the types in general and we need to hide the error message related with Java types. It is also true that we also need to make such logics in to Scala one to deduplicate this logic if they are duplicated. R has also a similar problem in some places. I don't think we should change this case anyway.

It looks we should ...

py4j.Py4JException: Method substr([class java.lang.Long ...

or we should introduce bridge methods in Scala side and implement this checking logic IIRC.

@HyukjinKwon HyukjinKwon Aug 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the latter, It looks we should call either substr with column,column or with int,int. I would like to avoid changing these If either way does not reduce the code diff and is virtually same, if I understood correctly.

@gatorsmile gatorsmile Aug 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ueshin

@ueshin ueshin Aug 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for the delay.
I guess we can support long by casting to int and also the "mixed" cases @gatorsmile metioned.
What do you think @HyukjinKwon ?

@HyukjinKwon HyukjinKwon Aug 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think we could support long. I think this PR basically targets exception message fix. Could we make this separate?

I guess supporting the case above requires a set of regression tests with min/max of int, fix for documentation and etc, which I think is rather loosely related with the JIRA.

.format(
startPos_t=type(startPos),
length_t=type(length),
))
if isinstance(startPos, (int, long)):

@HyukjinKwon HyukjinKwon Aug 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nchammas, supporting long with Python 2 is not documented in the docstring and looks we throw unexpected exception by long with Python 2 as below:

from pyspark.sql import Row
df = spark.createDataFrame([Row(name=u'Tom', height=80), Row(name=u'Alice', height=None)])
df.select(df.name.substr(long(1), long(3)).alias("col")).collect()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../spark/python/pyspark/sql/column.py", line 411, in substr
    jc = self._jc.substr(startPos, length)
  File ".../spark/python/lib/py4j-0.10.6-src.zip/py4j/java_gateway.py", line 1160, in __call__
  File ".../spark/python/pyspark/sql/utils.py", line 63, in deco
    return f(*a, **kw)
  File ".../spark/python/lib/py4j-0.10.6-src.zip/py4j/protocol.py", line 324, in get_return_value
py4j.protocol.Py4JError: An error occurred while calling o47.substr. Trace:
py4j.Py4JException: Method substr([class java.lang.Long, class java.lang.Long]) does not exist
	at py4j.reflection.ReflectionEngine.getMethod(ReflectionEngine.java:318)
	at py4j.reflection.ReflectionEngine.getMethod(ReflectionEngine.java:326)
	at py4j.Gateway.invoke(Gateway.java:274)
	at py4j.commands.AbstractCommand.invokeMethod(AbstractCommand.java:132)
	at py4j.commands.CallCommand.execute(CallCommand.java:79)
	at py4j.GatewayConnection.run(GatewayConnection.java:214)
	at java.lang.Thread.run(Thread.java:745)

Would you mind double checking this and taking long out with a simple test with Python 2 as well? I think this will also address @gatorsmile's concern above as well.

jc = self._jc.substr(startPos, length)
elif isinstance(startPos, Column):
Expand Down