Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
Agreed with @HyukjinKwon. This PR has a very narrow goal -- improving the
error messages -- which I think it accomplished. I think @gatorsmile was
expecting a more significant set of improvements,
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
The current codes around what this PR changes look not quite clean to me
too and we should clean around this.
But I think this PR itself is quite well-formed with the fix that is valid,
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18926
To be honest, the current codes do not look good to me. Since this does not
make the code worse, I will not revert it back.
---
If your project is set up for it, you can reply to this email and
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
Merged to master.
Please open JIRAs / PRs related with the discussion above if anyone is
willing to proceed.
---
If your project is set up for it, you can reply to this email and have
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
I am merging this as it looks there is an explicit objection for the
current change itself and it looks the issue is fixed by this.
To summarize the discussion here:
- Cleaning
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
I don't think this suggestion / discussion blocks this PR for few days.
Let's go as is and make a followup as another improvement if anyone feels so. I
will review that at my best.
---
If your
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
It's cleaner but less specific. Unless we branch on whether `startPos` and
`length` are the same type, we will give the same error message for mixed types
and for unsupported types. That seems like
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18926
```Python
if isinstance(startPos, int) and isinstance(length, int):
jc = self._jc.substr(startPos, length)
elif isinstance(startPos, Column) and isinstance(l
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
e
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80683/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80683 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80683/testReport)**
for PR 18926 at commit
[`a7fea20`](https://github.com/apache/spark/commit/a
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80683 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80683/testReport)**
for PR 18926 at commit
[`a7fea20`](https://github.com/apache/spark/commit/a7
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
@gatorsmile
> Even if we plan to drop `long` in this PR
We are not dropping `long` in this PR. It was [never
supported](https://github.com/apache/spark/pull/18926#discussion_r1328
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
> Basically, the change just wants to ensure the type of length is int.
Yes, but to be more correct, I think this makes sure if both are same
types, `Column` or `int`. I think this throw
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18926
Even if we plan to drop `long` in this PR, [the
checking](https://github.com/nchammas/spark/blob/fc1d84f002f5bd66bcad038a5581a05ade8dbc35/python/pyspark/sql/column.py#L408)
looks weird to me. Bas
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
For ^, I want to make this separate if possible. Do you guys strongly feel
about supporting `long` (and namely "mixed" types) here - @gatorsmile and
@ueshin?
---
If your project is set up for
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/18926
It sounds like the comment hides. Could you address the comment
https://github.com/apache/spark/pull/18926#discussion_r133121408?
---
If your project is set up for it, you can reply to this emai
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
LGTM except for the comment above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enab
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
I think my latest commits address the concerns raised here. Let me know if
I missed or misunderstood anything.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80643/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
e
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80643 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80643/testReport)**
for PR 18926 at commit
[`fc1d84f`](https://github.com/apache/spark/commit/f
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80643 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80643/testReport)**
for PR 18926 at commit
[`fc1d84f`](https://github.com/apache/spark/commit/fc
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
To summarize the feedback from @HyukjinKwon and @gatorsmile, I think what I
need to do is:
* Add a test for the mixed type case.
* Explicitly check for `long` in Python 2 and throw a `TypeEr
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
I was thinking of adding it in `python/pyspark/sql/tests.py`. Just in
case.. maybe we could add it around
https://github.com/apache/spark/commit/224e0e785b4b449ea638c2629263c798116a3011.
---
I
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
Oh, like a docstring test for the type error?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feat
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18926
Thank for cc'ing me. Yea looks fine. Could we add the small test in the
description just in case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user nchammas commented on the issue:
https://github.com/apache/spark/pull/18926
Pinging freshly minted committer @HyukjinKwon for a review on this tiny PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your p
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
e
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18926
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80544/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80544 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80544/testReport)**
for PR 18926 at commit
[`753dbe1`](https://github.com/apache/spark/commit/7
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18926
**[Test build #80544 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80544/testReport)**
for PR 18926 at commit
[`753dbe1`](https://github.com/apache/spark/commit/75
32 matches
Mail list logo
