[SPARK-21485][FOLLOWUP][SQL][DOCS] Describes examples and arguments separately, and note/since in SQL built-in function documentation

[HyukjinKwon]

What changes were proposed in this pull request?

This PR proposes to separate extended into examples and arguments internally so that both can be separately documented and add since and note for additional information.

For since, it looks users sometimes get confused by, up to my knowledge, missing version information. For example, see https://www.mail-archive.com/user@spark.apache.org/msg64798.html

For few good examples to check the built documentation, please see both:
from_json - https://spark-test.github.io/sparksqldoc/#from_json
like - https://spark-test.github.io/sparksqldoc/#like

For DESCRIBE FUNCTION, note and since are added as below:

> DESCRIBE FUNCTION EXTENDED rlike;
...
Extended Usage:
    Arguments:
      ...

    Examples:
      ...

    Note:
      Use LIKE to match with simple string pattern

> DESCRIBE FUNCTION EXTENDED to_json;
...
    Examples:
      ...

    Since: 2.2.0

For the complete documentation, see https://spark-test.github.io/sparksqldoc/

How was this patch tested?

Manual tests and existing tests. Please see https://spark-test.github.io/sparksqldoc

Jenkins tests are needed to double check

[HyukjinKwon]

I added since for both from_json and to_json as examples here. These were added into SQL since 2.2.0 - (https://issues.apache.org/jira/browse/SPARK-19637 and https://issues.apache.org/jira/browse/SPARK-19967)

[HyukjinKwon]

I believe we are okay to change this -

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/package.scala

Line 22 in 425c4ad

* considered an internal API to Spark SQL and are subject to change between minor releases.

and I could not find this in documentation.

[SparkQA]

Test build #79999 has finished for PR 18749 at commit 0958c0b.

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

[SparkQA]

Test build #80002 has finished for PR 18749 at commit d9277bf.

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

[SparkQA]

Test build #80003 has finished for PR 18749 at commit cb2dfe7.

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

[SparkQA]

Test build #80004 has finished for PR 18749 at commit 54e6d82.

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

[HyukjinKwon]

cc @rxin, @srowen and @cloud-fan, I believe this one is ready for a review. Could you take a look when you have some time?

[srowen]

There aren't many ExpressionInfo objects in memory right? adding more info to this bean doesn't have any meaningful performance implications, I presume. I suppose it's just breaking down the existing info further.

I also presume this is considered an internal API so it's OK to change the constructor. You could even retain the one constructor that is removed, just in case.

[HyukjinKwon]

Yes, I think so for the former and sure will keep the original constructor.

[srowen]

Agree, that's my only question, whether this change matters, because it's a developer API. You provide default implementations, though extended() gets removed. Hm. I am wondering if it's possible to keep extended() but, well, ignore it? it would at least be compatible even if it meant someone's implementation out there would have to update to provide information to ExpressionInfo correctly. That's not really a functional problem though.

[HyukjinKwon]

Will give a try.

[HyukjinKwon]

Will address comments within few days. (I am reading docs just to get used to things around my updated status)

[HyukjinKwon]

I manually tested source compatibility and I guess it also won't break binary compatibility - https://stackoverflow.com/questions/21197255/is-adding-a-method-to-a-java-annotation-safe-for-backward-compatibility

[HyukjinKwon]

In the last commit bf48875, I tried to keep the original behaviour and compatibility (although I guess we are fine to break this as said in few comments above):

For ExpressionDescription and ExpressionInfo,

if extended is an empty string (the default value), it creates the extended made by arguments, examples, note and since and use this as the extended description.

if extended is a non-empty string, it ignores the new arguments, examples, note and since but use this as the extended description, assuming this was set manually.

[SparkQA]

Test build #80044 has finished for PR 18749 at commit f51ee74.

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

[SparkQA]

Test build #80043 has finished for PR 18749 at commit bf48875.

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

[HyukjinKwon]

For source compatibility, in the commit, 389fc6e, I tried to move back to extended to show it passes the tests and keep the source compatibility.

For binary compatibility, I tested ExpressionDescription (which I guess we are worried of) in the way I described before - databricks/scala-style-guide#46.

With ExpressionDescription.java assuming this is Spark 2.2.0:

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface ExpressionDescription {
    String usage() default "";
    String extended() default "";
}

With Main.scala assuming this is the user application compiled with Spark 2.2.0:

@ExpressionDescription(
  usage = "I am usage",
  extended = """
    I am extended
  """)
case class A()

object Main {
  def main(args: Array[String]): Unit = {
    val df = scala.reflect.classTag[A].runtimeClass.getAnnotation(classOf[ExpressionDescription]).
    println(df.usage())
    println(df.extended())
  }
}

javac ExpressionDescription.java
scalac Main.scala
scala Main

prints

I am usage

    I am extended

And then, I changed ExpressionDescription.java assuming we merged this change into Spark's next release:

 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 @Retention(RetentionPolicy.RUNTIME)
 public @interface ExpressionDescription {
     String usage() default "";
     String extended() default "";
+    String arguments() default "";
+    String examples() default "";
+    String note() default "";
+    String since() default "";
 }

and then I ran

javac ExpressionDescription.java
scala Main

this looks working fine as below:

I am usage

    I am extended

[HyukjinKwon]

I will revert 389fc6e if the test passes.

[SparkQA]

Test build #80111 has finished for PR 18749 at commit 389fc6e.

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

[SparkQA]

Test build #80113 has finished for PR 18749 at commit f76af78.

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

[SparkQA]

Test build #80115 has finished for PR 18749 at commit 980ede7.

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

[SparkQA]

Test build #80116 has finished for PR 18749 at commit 1fa02c8.

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

[HyukjinKwon]

Now I am pretty confident for the compatibility concern given:

• the commit, 389fc6e (and with a little additional change, 1fa02c8), where I intendedly left extendeds as are in ExpressionDescription and ExpressionInfo usage, instead of setting new examples and arguments by the change proposal here to check the source compatibility.

• the manual tests, [SPARK-21485][FOLLOWUP][SQL][DOCS] Describes examples and arguments separately, and note/since in SQL built-in function documentation #18749 (comment), above to check binary compatibility.

I am quite sure if the current state should be ready for another look if the last test passes, where I reverted the former, 389fc6e, above. I guess it should pass because the only diff is 1fa02c8 but will wait for the results before pinging again.

[SparkQA]

Test build #80118 has finished for PR 18749 at commit b037592.

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

[HyukjinKwon]

retest this please

[SparkQA]

Test build #80123 has finished for PR 18749 at commit b037592.

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

[HyukjinKwon]

@srowen, @rxin and @cloud-fan, I am confident for the compatibility concern per #18749 (comment) and believe it is ready for another look.

[rxin]

What is the compatibility concern?

[HyukjinKwon]

Here, both comments by Sean, #18749 (comment) and #18749 (comment).

It looks we are okay to change this given #18749 (comment) but it is said in the comment, "DeveloperApi". So, I tried to keep the compatibility at my best.

So, I tried to think both possible compatibility cases:

• Checking If existing usage by developer still works with this change:

   @ExpressionDescription(
      usage = "...",
      extended = """
        ...
      """)

  The current PR changes extended to examples and arguments in its usage. So, I manually reverted this usage and checked if it passes the test.

  In other words, the tests pass with this change, and with both usages:

   @ExpressionDescription(
      usage = "...",
      arguments = """,
        ...
      """
      examples = """,
        ...
      """)

  and

   @ExpressionDescription(
      usage = "...",
      extended = """
        ...
      """)

• Checking binary compatibility by annotation. I added few element in this PR, examples, arguments, note and since. I manually checked this in [SPARK-21485][FOLLOWUP][SQL][DOCS] Describes examples and arguments separately, and note/since in SQL built-in function documentation #18749 (comment).

Did I maybe misunderstand something?

[rxin]

OK great. I think we should avoid breaking developer APIs, unless it has a huge upside. It wouldn't be fun to break it just for some cosmetic things ...

[SparkQA]

Test build #80248 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

retest this please

[SparkQA]

Test build #80250 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

retest this please

[gatorsmile]

We have to define a clear rule. We do not want to revert the PRs merged by others. It just makes others awkward.

In the past, based on my observation in the Spark SQL, when the committers left SGTM or looks good to me, it does not mean the quality is good enough for merging. That only means the quality or solution looks OK but needs more reviews/changes. I am not sure how the other Spark components work. However, I believe this is the rule we follow in Spark SQL. That is why I am surprised some committers merge the PR without an explicit LGTM message.

[srowen]

We have never had such a rule here or in any other project. Let's use common sense. If someone says 'looks good' that's fine. Or anything equivalent. If in doubt ask.

[gatorsmile]

In Spark SQL, we require such an explicit message. That is my understanding. If we want a change, I think we need to send a note to all the committers and let them know it. Then, they will leave such messages more carefully.

Spark SQL becomes more and more complex. It is very easy to break the backward compatibility or even return an incorrect result. We are building infrastructure software. We have to be more careful, because our current test case coverage is still not good enough.

[HyukjinKwon]

@gatorsmile, I really don't want to go through your PRs and I have not ever done such things i the past. Let's document this and then let me follow. Please open a discussion in a mailing list if you are in doubt. We are not supposed to make a long term decision.

[srowen]

@gatorsmile you are the one asking for a change, and I do not see support for it. I am not aware of any Spark SQL-specific rule; what are you referring to? Reynold agrees above, for example, that in some cases it's justified to merge a change with no other review. This alone suggests there is no rule about typing "LGTM".

You rightly say that things need review. The important change, the more it needs review. That is not an argument for a voting rule; it's an argument for good judgment among committers, and I see no evidence that's lacking. An "LGTM" rule doesn't solve that problem: a committer with bad judgment can always approve something for merging that shouldn't, right?

I further can't understand what's special about "LGTM". You are saying that committers say "sounds good to me" when they mean "this is not ready for merging" but whenever they say "LGTM" they always mean "this is ready for merging" -- sorry, what?

You are suggesting a change to a form of RTC, which would require a PMC vote. Consider the implications: nobody can merge a change until a vote completes. Are you sure?

In the meantime, come now, let's move ahead with this PR, which was obviously reviewed.

[rxin]

@srowen that's not what I said. Almost always an explicit LGTM from somebody familiar with the codebase is preferred. There are tiny changes that might not require them, and it is up to the judgement of the committer. But those are more exceptions than the norm.

[srowen]

Yep, I agree with 'almost always', and tiny changes might not require them. I think that's what I said you said? "some cases". See also #18749 (comment)

But this alone disagrees with the idea that all changes need the string "LGTM" typed.

I am not sure why this popped up, but I propose we continue with the same process and conventions we always have. If anyone's unsure about whether an important review is affirmative: please ask.

Can we put this to bed now?

[SparkQA]

Test build #80253 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

retest this please

[gatorsmile]

I am also afraid somebody might misunderstand what I or a few other committers say SGTM or looks good in Spark SQL. When a committer tries to merge Spark SQL related PRs based on these messages, please double checks. Does this sound reasonable?

[SparkQA]

Test build #80261 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

retest this please

[SparkQA]

Test build #80274 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

retest this please

[SparkQA]

Test build #80282 has finished for PR 18749 at commit 974eab2.

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

[HyukjinKwon]

Other test failures look spurious and unrelated. I guess we are okay to go ahead with this PR anyway?

[gatorsmile]

LGTM.

Thanks! Merging to master.