Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add deprecated function warning #2446

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Add deprecated function warning #2446

merged 1 commit into from
Jan 22, 2020

Conversation

oneonestar
Copy link
Member

@oneonestar oneonestar commented Jan 9, 2020 •
edited
Loading

Generate a warning when a query is using a @Deprecated function.
This works for annotated Scalar, Parametric Scalar, Aggregation, Window functions.
Both built-in UDFs and UDFs from plugin are supported.
Doesn't work for classes that are directly implements SqlFunction without using annotation.

This feature can be enabled by the feature flag analyzer.query-diagnosis-warning-enabled or enable_query_diagnosis_warning session. Default is not enabled.

I have two questions:

  • Is the feature flag needed?
  • Should this function be default ON / OFF?
@Deprecated
@ScalarFunction("deprecated_function")
public static Slice funcABC() { ... }
presto> SELECT deprecated_function();
.....
(1 row)

WARNING: Detected use of deprecated function: deprecated_function

Supersedes #1006

@cla-bot cla-bot bot added the cla-signed label Jan 9, 2020
@oneonestar oneonestar requested a review from ebyhr January 9, 2020 01:04
@oneonestar oneonestar mentioned this pull request Jan 9, 2020
Closed
@oneonestar oneonestar self-assigned this Jan 9, 2020
@findepi findepi added the enhancement New feature or request label Jan 9, 2020
martint
martint requested changes Jan 17, 2020
View reviewed changes
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a good start, but I have some comments about the approach.

...o-main/src/main/java/io/prestosql/sql/planner/sanity/warnings/DeprecatedFunctionWarning.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/SystemSessionProperties.java Outdated Show resolved Hide resolved
...o-main/src/main/java/io/prestosql/sql/planner/sanity/warnings/DeprecatedFunctionWarning.java Outdated Show resolved Hide resolved
presto-spi/src/main/java/io/prestosql/spi/connector/StandardWarningCode.java Outdated Show resolved Hide resolved
@martint martint self-requested a review January 17, 2020 22:57
mosabua
mosabua reviewed Jan 20, 2020
View reviewed changes
presto-docs/src/main/sphinx/develop/functions.rst Outdated
Deprecated Function
-------------------

The ``@Deprecated`` annotation can be used to mark a function as deprecated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecated. It should

mosabua
mosabua reviewed Jan 20, 2020
View reviewed changes
presto-docs/src/main/sphinx/develop/functions.rst Outdated
-------------------

The ``@Deprecated`` annotation can be used to mark a function as deprecated
and should no longer be used. Presto will generates a warning whenever a SQL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presto generates

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and SQL statement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and SQL statement

You mean a SQL statement?

-The ``@Deprecated`` annotation can be used to mark a function as deprecated
-and should no longer be used. Presto will generates a warning whenever a SQL
+The ``@Deprecated`` annotation can be used to mark a function as deprecated.
+It should no longer be used. Presto generates a warning whenever a SQL statement

@mosabua
Copy link
Member

mosabua commented Jan 20, 2020

Thank you for that contribution. This is great.

In terms of rolling this out to users it might make sense to do the following:

  • ship with feature flag and default to OFF for a release or two, and document this in release notes and normal documentation
  • ship with feature flag and default to ON afterwards for better overall usability
  • potentially remove feature flag and leave always on after depending on community input

@mosabua
Copy link
Member

mosabua commented Jan 20, 2020

Also .. I review the doc based on request from @martint

martint
martint requested changes Jan 21, 2020
View reviewed changes
presto-tests/src/test/java/io/prestosql/execution/TestDeprecatedFunctionWarning.java Outdated Show resolved Hide resolved
presto-tests/src/test/java/io/prestosql/execution/TestDeprecatedFunctionWarning.java Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/develop/functions.rst Outdated
-------------------

The ``@Deprecated`` annotation can be used to mark a function as deprecated.
It should no longer be used. Presto generates a warning whenever a SQL statement
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little ambiguous. It's not clear whether it refers to the function or the annotation. Maybe rephrase as:

The @Deprecated annotation can be used to mark a function as deprecated and to indicate that it should no longer be used.

@mosabua, do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha.. that was my suggestion .. I thought it is clear as one sentence.
Maybe we use

The @Deprecated annotation has to be used on any function that should no longer be used. The annotation causes Presto to generate a warning whenever...

presto-main/src/main/java/io/prestosql/SystemSessionProperties.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JsonFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved
presto-tests/src/test/java/io/prestosql/execution/TestUDFPluginDeprecatedUDFPlugin.java Outdated Show resolved Hide resolved
martint
martint approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good -- just one minor thing I missed before. Can you squash the commits? I'll merge it once you do that.

presto-testing/src/main/java/io/prestosql/testing/DistributedQueryRunner.java Outdated Show resolved Hide resolved
@oneonestar oneonestar mentioned this pull request Jan 22, 2020
Closed
@martint martint merged commit 8e242e8 into trinodb:master Jan 22, 2020
@martint
Copy link
Member

martint commented Jan 22, 2020

Merged, thanks!

@oneonestar oneonestar deleted the deprecated_warn branch January 23, 2020 00:31
@findepi findepi added this to the 329 milestone Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua left review comments

@martint martint martint approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees

@oneonestar oneonestar

Labels
cla-signed enhancement New feature or request ready to merge
Milestone
329
Development

Successfully merging this pull request may close these issues.
4 participants
@oneonestar @mosabua @martint @findepi