[SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for extended usage and SQL doc #24259

HyukjinKwon · 2019-03-31T06:26:16Z

What changes were proposed in this pull request?

This PR proposes to two things:

Add deprecated field to ExpressionDescription so that it can be shown in our SQL function documentation (https://spark.apache.org/docs/latest/api/sql/), and it can be shown via DESCRIBE FUNCTION EXTENDED.
While I am here, add some more restrictions for note() and since(). Looks some documentations are broken due to malformed note:

It should start with 4 spaces and end with a newline. I added some asserts, and fixed the instances together while I am here. This is technically a breaking change but I think it's too trivial to note somewhere (and we're in Spark 3.0.0).

This PR adds deprecated property into from_utc_timestamp and to_utc_timestamp (it's deprecated as of #24195) as examples of using this field.

Now it shows the deprecation information as below:

SQL documentation is shown as below:

DESCRIBE FUNCTION EXTENDED from_utc_timestamp;:

Function: from_utc_timestamp
Class: org.apache.spark.sql.catalyst.expressions.FromUTCTimestamp
Usage: from_utc_timestamp(timestamp, timezone) - Given a timestamp like '2017-07-14 02:40:00.0', interprets it as a time in UTC, and renders that time as a timestamp in the given time zone. For example, 'GMT+1' would yield '2017-07-14 03:40:00.0'.
Extended Usage:
    Examples:
      > SELECT from_utc_timestamp('2016-08-31', 'Asia/Seoul');
       2016-08-31 09:00:00

    Since: 1.5.0

    Deprecated:
      Deprecated since 3.0.0. See SPARK-25496.

How was this patch tested?

Manually tested via:

For documentation verification:
```
$ cd sql
$ sh create-docs.sh
```

For checking description:

$ ./bin/spark-sql

spark-sql> DESCRIBE FUNCTION EXTENDED from_utc_timestamp;
spark-sql> DESCRIBE FUNCTION EXTENDED to_utc_timestamp;

rxin · 2019-03-31T06:40:32Z

The issue here is that this isn't quite sufficient, because users don't always read docs (as a matter of fact, for people that have something working, they don't need to look up the docs to get it work), and SQL users also don't see the warning messages in logs.

SparkQA · 2019-03-31T07:05:01Z

Test build #104133 has finished for PR 24259 at commit ba4f976.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-03-31T09:31:25Z

Thanks, @rxin. I agree that practically users tend to don't read the doc although they are supposed to read the documentation. I was thinking it can be a matter that we officially document the deprecation or not in a way (for instance, I don't think so many users would read release docs or actual JIRAs often ..).

I quickly gave a rough try to add a warning with all default settings with spark-sql:

spark-sql> SELECT from_utc_timestamp(timestamp(1), "UTC");
19/03/31 18:23:40 WARN FromUTCTimestamp: Don't use from_utc_timestamp.
19/03/31 18:23:40 WARN FromUTCTimestamp: Don't use from_utc_timestamp.
19/03/31 18:23:43 INFO CodeGenerator: Code generated in 215.181876 ms
19/03/31 18:23:43 INFO SparkContext: Starting job: processCmd at CliDriver.java:376
19/03/31 18:23:43 INFO DAGScheduler: Got job 0 (processCmd at CliDriver.java:376) with 1 output partitions
19/03/31 18:23:43 INFO DAGScheduler: Final stage: ResultStage 0 (processCmd at CliDriver.java:376)
19/03/31 18:23:43 INFO DAGScheduler: Parents of final stage: List()

I think Scala, Python, SparkR users also see similar stuff. Yes, SQL itself can be different in its nature but I think we should better have another mechanism for users to encourage to read them in this case.

In the way of throwing an error (proposed and being discussed at #24195), I think it can be problematic in the future to manage all those configurations. For instance, currently it sounded like we will go ahead with this approach from now on. As we know, Spark has a lot of configurations and flags, and I am worried of the maintenance overhead (I remember that I read an extreme example of flags).

At least this PR encourages Spark developers to note deprecated SQL functions which can be accessed in runtime (as well as they are shown in SQL documentation).

HyukjinKwon · 2019-03-31T09:47:52Z

Or how about we have one configuration to enable and disable all deprecated SQL expressions? I think I can fix it here futher to add such mechanism if it's inevitable to throw an error.

srowen · 2019-03-31T14:03:40Z

I don't mind this support. I also think a WARN level message is pretty good.
For Spark 3 I'm not against removing this functionality entirely, though still have a slight preference for just deprecating it as much as is possible.

cloud-fan · 2019-04-01T07:16:29Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionDescription.java

+ * for example, "2.2.0". It should not start with non-number characters.
+ *
+ * `deprecated()` contains deprecation information for the expression optionally, for example,
+ * "Deprecated since 2.2.0. Use something else instead". This property should have 4


not related to this PR, but why do we have such a weird requirement for the text?

To make it pretty in SQL documentation. Actually, we can loose the requirement by adding some logics to handle in gen-sql-markdown.py; however, I thought it's better to leave it strict to have uniformed code style.

cloud-fan · 2019-04-01T07:17:26Z

I'm fine with this approach, with the proposal have one configuration to enable and disable all deprecated SQL expressions. The config can be added with another PR though.

HyukjinKwon · 2019-04-01T07:43:14Z

WDYT, @rxin and @srowen?

My main concern is that we happen to start to have configuration for each deprecation, ending up with many configurations in the future. I prefer to don't throw an error as deprecation but I think I am fine since that's not my main concern.

HyukjinKwon · 2019-04-01T07:53:53Z

sql/gen-sql-markdown.py

+    """
+
+    if deprecated != "":
+        deprecated = "\n".join(map(lambda n: n[4:], deprecated.split("\n")))


@cloud-fan, for instance this logic matters.

If we put some strings in ..

@ExpressionDescription( ... deprecated = """ blah blah blah I am super long line 1. one 2. I am markdwn - bullet marks - aa You know what? actually you can write a markdown table as well. | Tables | Are | Cool | | ------------- |:-------------:| -----:| | col 3 is | right-aligned | $1600 | | col 2 is | centered | $12 | """)

gen-sql-markdown.py here split the doc into each line and then get rid of the first 4 characters for each. Last newline makes a proper indentation between each field.

will people use multi-line string literal in Scala? e.g.

deprecated = """ |xxx |xxx """.stripMargin

That cannot be used here due to compilation error. It seems annotation parameters only allow constants. There was a discussion about it before when I first add examples.

what if the expression is inside an object? e.g.

object A { @ExpressionDescription( note = """ blabla """ ) class B }

Then the string has 6 spaces at the beginning.

It looks to me that we can relax the format a little bit: just trim all the spaces at the beginning. Then we can even use a single line string.

Yes, in that case it might be a problem although there look no case so far after this fix (I skimmed the built doc).

The thing we should take into account is we shouldn't trim all the white spaces all because spacing matters in md rendering. For instance, if we have a doc like ..

object A { @ExpressionDescription( note = """ blabla - first blabla - second blabla """ ) class B }

We should only trim leading 6 spaces for each line. So, it will bring a bit of complication. It's doable though.

If you don't mind, I will handle it later. Was thinking it better have a single format for now. We should encourage to have more verbose and detailed doc anyway (rather than single line doc) in general.

ah that makes sense, it's more tricky than I thought...

rxin · 2019-04-02T00:54:36Z

So the issue with a single flag for all the deprecated functions is that the more functions we deprecate, the more likely one will just turn it on always, and as a result it becomes completely useless. Why do you think it's a lot of overhead to add new flags? Isn't it just a few lines of code? We don't need to define an extra function in the SQLConf object.

HyukjinKwon · 2019-04-02T01:31:39Z

It is a few pieces of codes, yes. I was worrying about we start to have many flags under SQLConf (where we already have a lot). From what I have seen, it always grows but never decreases.

Adding a configuration for each deprecation with throwing an exception looked like it makes harder to deprecate and get out of the legacy stuff. (for instance, the SparkR and PySpark tests had to be fixed few times in #24195).

HyukjinKwon · 2019-04-02T01:36:11Z

Or, how about adding a configuration that lists up deprecated functions? In this way, everything could be fine I suspect.

cloud-fan · 2019-04-02T01:38:50Z

Another point of view: how often do we deprecate SQL functions? from/to_utc_timestamp is the first one and I don't know if we will deprecate more. Seems like it's better to do generalization when we hit something more than once.

HyukjinKwon · 2019-04-02T01:41:49Z

My impression was that it's been difficult to deprecate SQL functions so it has not been deprecated often (I suspect there might be some more cases that we should deprecate).

But .. okay ... we can revisit this when we see this issue again.

BTW, the PR itself here can be merged orthogonally I believe anyway since it just adds some more notes for SQL function deprecation.

cloud-fan · 2019-04-02T01:46:53Z

BTW, the PR itself here can be merged orthogonally I believe anyway since it just adds some more notes for SQL function deprecation.

+1, it makes the generated doc better

rxin · 2019-04-02T02:22:53Z

Sorry guys your arguments don't really hold. If we deprecate only a small number of expressions, why does it matter than for each of them we'd need to add a few lines? If we don't want to pollute SQLConf because it is getting long, we can create a new object called SQLConfDeprecated or something like that.

A single global deprecation flag doesn't work. Imagine a library you use that uses one of those deprecated configs. Now you will never get any warning anywhere ...

cloud-fan · 2019-04-02T02:32:18Z

@rxin yea we will go with the "fail by default, with a config to fallback" approach. FYI #24195 has been merge.

This PR is just to add a new entry in the generated SQL function doc, to indicate that a function is deprecated since version X.

HyukjinKwon · 2019-04-02T02:41:16Z

Yes, this one just adds some more info about deprecation. For conf with failing, I'm okie with going ahead for now and talking about that later when we see it's often.

…s documentation

SparkQA · 2019-04-02T06:23:15Z

Test build #104180 has finished for PR 24259 at commit ba4adb2.

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

SparkQA · 2019-04-02T13:53:23Z

Test build #104201 has finished for PR 24259 at commit 1db146c.

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

SparkQA · 2019-04-02T14:10:09Z

Test build #104202 has finished for PR 24259 at commit f4cd97f.

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

SparkQA · 2019-04-02T14:12:43Z

Test build #104203 has finished for PR 24259 at commit 4736081.

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

HyukjinKwon · 2019-04-03T03:22:55Z

retest this please

SparkQA · 2019-04-03T07:05:02Z

Test build #104228 has finished for PR 24259 at commit 4736081.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-04-03T07:31:49Z

retest this please

SparkQA · 2019-04-03T11:42:28Z

Test build #104238 has finished for PR 24259 at commit 4736081.

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

HyukjinKwon · 2019-04-03T12:06:06Z

Hmmmhm? I haven;t seen such test failures before.

HyukjinKwon · 2019-04-03T12:06:13Z

retest this please

SparkQA · 2019-04-03T16:15:24Z

Test build #104245 has finished for PR 24259 at commit 4736081.

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

nandorKollar · 2019-04-04T16:09:01Z

@HyukjinKwon I'm not familiar with Python, so not sure that this is helpful. :) Isn't this failure related somehow to this? Was pytz version updated in this node? I could see some version updates here:

The following NEW packages will be INSTALLED:

    blas:            1.0-mkl                
    ca-certificates: 2019.1.23-0            
    certifi:         2018.8.24-py35_1       
...
    pytz:            2018.5-py35_0

but these are related to Python 3, and the test failed with 2.

srowen · 2019-04-04T16:13:56Z

See #24266

HyukjinKwon · 2019-04-08T13:34:48Z

retest this please

SparkQA · 2019-04-08T18:40:18Z

Test build #104388 has finished for PR 24259 at commit 4736081.

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

HyukjinKwon · 2019-04-08T20:53:10Z

I think this is ready for a look.

cloud-fan · 2019-04-09T03:53:15Z

LGTM except one comment: https://github.com/apache/spark/pull/24259/files#r273316356

cloud-fan · 2019-04-09T05:48:07Z

thanks, merging to master!

HyukjinKwon · 2019-04-09T06:45:36Z

Thanks all!

HyukjinKwon mentioned this pull request Mar 31, 2019

[SPARK-25496][SQL] Deprecate from_utc_timestamp and to_utc_timestamp #24195

Closed

HyukjinKwon changed the title ~~[SPARK-27328][SQL] Create 'deprecate' property in ExpressionDescription for SQL function documentation~~ [SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for SQL function documentation Mar 31, 2019

HyukjinKwon changed the title ~~[SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for SQL function documentation~~ [SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for extended usage and SQL doc Mar 31, 2019

cloud-fan reviewed Apr 1, 2019

View reviewed changes

HyukjinKwon commented Apr 1, 2019

View reviewed changes

HyukjinKwon added 2 commits April 2, 2019 12:34

Create 'deprecate' property in ExpressionDescription for SQL function…

071825d

…s documentation

Add deprecated fields to both from_utc_timestamp and to_utc_timestamp

ba4adb2

HyukjinKwon force-pushed the SPARK-27328 branch from ba4f976 to ba4adb2 Compare April 2, 2019 03:56

HyukjinKwon added 3 commits April 2, 2019 17:58

Make it pretty

1db146c

Lose condition

f4cd97f

Fix a typo

4736081

cloud-fan closed this in f16dfb9 Apr 9, 2019

HyukjinKwon deleted the SPARK-27328 branch March 3, 2020 01:18

This was referenced Apr 10, 2020

[SPARK-31349][SQL][DOCS] Document built-in aggregate functions in SQL Reference #28120

Closed

[SPARK-31369][SQL][DOCS] Documentation for JSON Functions #28170

Closed

[SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for extended usage and SQL doc #24259

[SPARK-27328][SQL] Add 'deprecated' in ExpressionDescription for extended usage and SQL doc #24259

Uh oh!

Conversation

HyukjinKwon commented Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

rxin commented Mar 31, 2019

Uh oh!

SparkQA commented Mar 31, 2019

Uh oh!

HyukjinKwon commented Mar 31, 2019

Uh oh!

HyukjinKwon commented Mar 31, 2019

Uh oh!

srowen commented Mar 31, 2019

Uh oh!

cloud-fan Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 1, 2019

Uh oh!

HyukjinKwon commented Apr 1, 2019

Uh oh!

HyukjinKwon Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 9, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 9, 2019

Choose a reason for hiding this comment

Uh oh!

rxin commented Apr 2, 2019

Uh oh!

HyukjinKwon commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Apr 2, 2019

Uh oh!

cloud-fan commented Apr 2, 2019

Uh oh!

HyukjinKwon commented Apr 2, 2019

Uh oh!

cloud-fan commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Apr 2, 2019

Uh oh!

cloud-fan commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

HyukjinKwon commented Apr 3, 2019

Uh oh!

SparkQA commented Apr 3, 2019

HyukjinKwon commented Mar 31, 2019 •

edited

Loading

HyukjinKwon Apr 9, 2019 •

edited

Loading

HyukjinKwon commented Apr 2, 2019 •

edited

Loading

cloud-fan commented Apr 2, 2019 •

edited

Loading

cloud-fan commented Apr 2, 2019 •

edited

Loading