Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
True. That is why I do not know which is the best way for us to show the
argument/parameter names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/15677
These function docs are printed onto the console.
---
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 fea
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Normally, most RDBMS docs are using _Italic fonts_ for the
parameter/argument names. Linux man is using `underscore` to highlight argument
names in descriptions. I also saw another way
```
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Some of them already had backricks in the description and others did not.
Matching it up with backticks was initially suggested by
https://github.com/apache/spark/pull/15513#discussion_r84820066
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/15677
@HyukjinKwon why did we add backticks to surround all parameters? It looks
pretty weird actually.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHu
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Merging to master and 2.1! Thanks!
---
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
enabl
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
(https://github.com/apache/spark/pull/15677#issuecomment-258050659 was on
the past commit. I resolved the conflicts twice.)
---
If your project is set up for it, you can reply to this email and
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/68027/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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
enabled
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #68027 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68027/consoleFull)**
for PR 15677 at commit
[`cd0c810`](https://github.com/apache/spark/commit/
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/68033/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
**[Test build #68033 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68033/consoleFull)**
for PR 15677 at commit
[`6ad2068`](https://github.com/apache/spark/commit/
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/68031/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #68031 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68031/consoleFull)**
for PR 15677 at commit
[`4ed9d03`](https://github.com/apache/spark/commit/
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
LGTM pending test.
---
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
enabled and wishes so
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #68033 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68033/consoleFull)**
for PR 15677 at commit
[`6ad2068`](https://github.com/apache/spark/commit/6
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Thanks @gatorsmile and @srowen. It seems ready.
---
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
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #68031 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68031/consoleFull)**
for PR 15677 at commit
[`4ed9d03`](https://github.com/apache/spark/commit/4
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #68027 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68027/consoleFull)**
for PR 15677 at commit
[`cd0c810`](https://github.com/apache/spark/commit/c
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
LGTM except one minor
[comment](https://github.com/apache/spark/pull/15677#discussion_r86185412).
Thanks for your work!
---
If your project is set up for it, you can reply to this ema
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Thanks for asking. I think yes it is ready.
---
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 fea
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile @HyukjinKwon do you both feel like this is ready to merge?
---
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
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67964/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
**[Test build #67964 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67964/consoleFull)**
for PR 15677 at commit
[`d25abca`](https://github.com/apache/spark/commit/
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67964 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67964/consoleFull)**
for PR 15677 at commit
[`d25abca`](https://github.com/apache/spark/commit/d
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67962/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67962 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67962/consoleFull)**
for PR 15677 at commit
[`b2fb5e6`](https://github.com/apache/spark/commit/
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Merged build finished. Test FAILed.
---
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/15677
**[Test build #67962 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67962/consoleFull)**
for PR 15677 at commit
[`b2fb5e6`](https://github.com/apache/spark/commit/b
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67946/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
**[Test build #67946 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67946/consoleFull)**
for PR 15677 at commit
[`a6b50eb`](https://github.com/apache/spark/commit/
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67946 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67946/consoleFull)**
for PR 15677 at commit
[`a6b50eb`](https://github.com/apache/spark/commit/a
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
I finished the last pass. Thanks for your work!
---
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
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67900/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67900 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67900/consoleFull)**
for PR 15677 at commit
[`0bfbaee`](https://github.com/apache/spark/commit/
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67892/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67892 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67892/consoleFull)**
for PR 15677 at commit
[`57153d6`](https://github.com/apache/spark/commit/
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67900 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67900/consoleFull)**
for PR 15677 at commit
[`0bfbaee`](https://github.com/apache/spark/commit/0
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67891 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67891/consoleFull)**
for PR 15677 at commit
[`7c2959d`](https://github.com/apache/spark/commit/
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Merged build finished. Test FAILed.
---
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/15677
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67891/
Test FAILed.
---
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
(PR description is updated too.) Thank you @srowen.
---
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
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67892 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67892/consoleFull)**
for PR 15677 at commit
[`57153d6`](https://github.com/apache/spark/commit/5
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Alright, then will try to get rid of the arguments part. Thank you all very
much sincerely and I apologise the noise I caused. @gatorsmile I will keep in
mind you comments too.
---
If your pro
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
@HyukjinKwon Sure, I can try it.
Actually, we might need to find multiple typical function APIs as examples
to discuss what are the best way to document/specify the argument/parameter
t
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Okay, @gatorsmile could you maybe submit a small one first please? Will
refer yours and then I can work together (we can divide the files or packages
to deal with I guess).
---
If your project
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/15677
BTW this is a cool change. We should try to get it in for 2.1.0.
---
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 no
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/15677
To be honest I myself would never submit such a large pr without strong
likelihood of it just getting committed in one or two shots; otherwise it is
just impossible to get it merged with conflicts pili
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
@srowen @HyukjinKwon I can submit PRs to add the argument sections and cc
you to review. However, I might not submit a huge PR for adding all of them in
the same one. Like what I said before, I w
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
I am fine and willing to follow the majority. We have anyway the same goal.
I hope the followup is open as soon as possible after this one though if we
decide to remove argument parts here.
--
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile Taking your example, before, the user gets no information at
all about the type, except that it looks maybe string-like. Now at least we say
it's a 'string expression' which suggests, cor
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
I did not say the other changes are wrong. The only concern is argument
types. Like what I said above, if we remove the newly added argument types, I
am fine to merge this as a document improveme
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
My answer to that is
https://github.com/apache/spark/pull/15677#issuecomment-257137604. It seems the
discussion goes in a loop.
---
If your project is set up for it, you can reply to this ema
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Sorry, I am confusing the merging standards. Why can we accept merging
something incorrect?
Just like `reverse`, to the external users, the argument type of `str` is
not `a string expre
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
I guess we can improve them in followups if a change does not make
something worse. This introduces a general format to follow too.
Can I just follow the majority if this can't be unanim
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
> However, the complex type we should block. We also need to fix it in the
code.
It does not support complex types already but only `AtomicType`[1].
```
spark-sql> SELECT re
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
https://github.com/apache/spark/pull/15690 is to resolve the behaviors of a
few string-to-string expressions, including `reverse`. After rethinking about
it, I think we should not support implici
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
IMO, when we add the argument types in the function descriptions, we should
deliver the corresponding test cases in the same PRs. Otherwise, I do not think
how we can know the argument types are
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
The argument would be: because it's not less-right? (I'm not even judging
whether your points are right but I presume they are.)
I think we did originally not have nearly this much change re
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
If the existing way to document argument types is not right, why we still
want to merge it into the master branch?
Why not removing them from this PR, if we care the PR grows too big?
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
That's fine, they can be changed soon after. Put it this way: do you feel
the same argument applies separately to the existing documentation? if it
wasn't a 'regression' in this PR i think it's separ
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
I am still uncomfortable about the newly added contents regarding the
argument types, if we do not resolve the above three general issues.
If this were merged, basically, in the follow-up
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile I suggest we handle all of that as a follow-on PR, because this
has already become a series of good related changes that has nevertheless
become a very big change. Each of those were exis
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Based on my review in this PR, I think we need to resolve the following
three major issues when we document the argument type:
- First, implicit type casting should be clearly explained w
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
```Scala
val df2 = Seq((2.0f, 345)).toDF("a", "b")
checkAnswer(
df2.select(reverse($"a"), reverse($"b")),
Row("0.2", "543"))
```
The argument of `re
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67779/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
**[Test build #67779 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67779/consoleFull)**
for PR 15677 at commit
[`b46404b`](https://github.com/apache/spark/commit/
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67779 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67779/consoleFull)**
for PR 15677 at commit
[`b46404b`](https://github.com/apache/spark/commit/b
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Could I please ask your opinion @rxin ? I would rather simply follow the
majority.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile, I guess the changes here are roughly correct and accurate and
make sense. This does not mean "not right" or "wrong".
For example, if users run `describe function extended r
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
How about splitting this PR to two parts? One is the document improvement
(fixing typos and adding examples). Another is to add argument descriptions
(many parts are not clearly defined. More dis
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
@srowen If the newly added contents are not right, should we commit them
and fix them later?
If this PR does not add argument descriptions, I am fine to merge it (after
one more pass) b
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile yes, this is why I suggested earlier that we stop and commit
the changes we had in the last PR. We have to break this up to make it
manageable in this case, because of the size. There are
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Not sure whether the other reviewers have the same feeling like me. 2000+
LOC is too big, imo. The focus of this PR should be on the newly added argument
types, descriptions and examples.
---
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile OK, so, your concern is about potentially inaccurate types and
typos. If you are really worried, maybe I can split this PR into several ones.
---
If your project is set up for it, y
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
I am feeling sorry, but I have to say I will not merge the PR if the
quality is not good enough. Maybe my standard is too high. If the other
Committers have different opinions, they can merge it
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
Hey @gatorsmile, I am not rushing to review and also I didn't mean to find
every one. I meant I can wait for all the reviews and then sweep it if they are
only minors and each does not block eac
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/15677
Since this PR is 2000+ line changes, I am unable to quickly go over all of
them. Normally, creating such a big PR is not welcomed, because it is hard for
reviewers to read throught all the change
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15677
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/15677
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67733/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67733 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67733/consoleFull)**
for PR 15677 at commit
[`2b437fe`](https://github.com/apache/spark/commit/
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
BTW, I hope we can fix up all the minor comments together once as a final
look if each does not block each other.
---
If your project is set up for it, you can reply to this email and have your
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
@gatorsmile I double-checked the type ones again and tried to describe the
types more specifically. Could you please take another look?
---
If your project is set up for it, you can reply to th
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15677
cc @srowen, @rxin, @jodersky, @gatorsmile . I closed the previous one and
reopened it here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHu
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15677
**[Test build #67733 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67733/consoleFull)**
for PR 15677 at commit
[`2b437fe`](https://github.com/apache/spark/commit/2
92 matches
Mail list logo
