[SPARK-33415][PYTHON][SQL] Don't encode JVM response in Column.__repr__ #30322
Conversation
|
|
||
| def __repr__(self): | ||
| return 'Column<%s>' % self._jc.toString().encode('utf8') | ||
| return "Column<'%s'>" % self._jc.toString() |
There was a problem hiding this comment.
python3 uses utf8 for strings by default, so this change seems fine. cc: @HyukjinKwon @viirya @srowen
There was a problem hiding this comment.
Do we have any more instances of decode()?
There was a problem hiding this comment.
Yeah, the change looks good, and yeah, are there more instances like this?
There was a problem hiding this comment.
Seems fine. Is it originally for non printable characters in unicode?
There was a problem hiding this comment.
Do we have any more instances of decode()?
We do a bit of encoding / decoding when we communicate with JVM, but purpose there is clear.
The only other place when we encode strings intended for user consumption is RDD.toDebugString. It also something that could be fixed, as it messing with the output a bit (as print won't respect line breaks).
Seems fine. Is it originally for non printable characters in unicode?
I believe the point was to have str object as the output, instead of unicode. If I recall correctly, unicode (py4j returns JVM Strings as unicode in Python 2 and as a result the whole expression would evaluate to unicode) in __repr__, wasn't handled correctly.
There was a problem hiding this comment.
For example in IPython
>>> import sys
>>> sys.version_info
sys.version_info(major=2, minor=7, micro=15, releaselevel='final', serial=0)
>>> class Foo:
... def __repr__(self):
... return u"œ"
...
...
>>> Foo().__repr__()
u'\u0153'
>>> Foo()
Traceback (most recent call last):
File "/path/to/lib/python2.7/site-packages/IPython/core/formatters.py", line 686, in __call__
return repr(obj)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u0153' in position 0: ordinal not in range(128)
|
Why WIP? btw, could you add tests? |
|
Test build #130892 has finished for PR 30322 at commit
|
|
Test build #130893 has finished for PR 30322 at commit
|
|
Kubernetes integration test starting |
Just out of habit after working on docs.
Of course, done. |
|
Test build #130894 has finished for PR 30322 at commit
|
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test status failure |
|
Let me merge this in for now as this PR fixes what it aims. Merged to master. |
|
Thanks everyone! |
6 participants
What changes were proposed in this pull request?
Removes encoding of the JVM response in
pyspark.sql.column.Column.__repr__.Why are the changes needed?
API consistency and improved readability of the expressions.
Does this PR introduce any user-facing change?
Before this change
result in
After this change we'll get
How was this patch tested?
Existing tests and manual inspection.