You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/02/20 07:49:48 UTC
[GitHub] [spark] dongjoon-hyun opened a new pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
dongjoon-hyun opened a new pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce any user-facing change?
<!--
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588724817
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595474878
**[Test build #119407 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119407/testReport)** for PR 27643 at commit [`1b497af`](https://github.com/apache/spark/commit/1b497afbdb165e39f89426deb2a39935fbb0ae61).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344056
**[Test build #119407 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119407/testReport)** for PR 27643 at commit [`1b497af`](https://github.com/apache/spark/commit/1b497afbdb165e39f89426deb2a39935fbb0ae61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn
two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588724755
**[Test build #118702 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118702/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r382116735
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1793,6 +1794,7 @@ class Analyzer(
* Replaces [[UnresolvedFunction]]s with concrete [[Expression]]s.
*/
object ResolveFunctions extends Rule[LogicalPlan] {
+ val trimWarningEnabled = new AtomicBoolean(true)
Review comment:
When do you set it back to `true`? The `ResolveFunctions` object can live forever. By setting it to `false` only once, you disable warnings for all apps and users.
Let's image multiple notebooks connected to the same cluster - they will share the same `trimWarningEnabled` if I am not wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595083120
**[Test build #119374 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119374/testReport)** for PR 27643 at commit [`e22b26e`](https://github.com/apache/spark/commit/e22b26ebdba19b45079fda80f29696ce6e6708d6).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun edited a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun edited a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588750339
Got it. I'll try tomorrow. BTW, do we have another test example for `log once per session` validation?
> shall we add a test using LogAppender? The expectation is log once per session.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588712511
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23453/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388699173
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
##########
@@ -768,4 +769,54 @@ class AnalysisSuite extends AnalysisTest with Matchers {
assert(message.startsWith(s"Max iterations ($maxIterations) reached for batch Resolution, " +
s"please set '${SQLConf.ANALYZER_MAX_ITERATIONS.key}' to a larger value."))
}
+
+ test("SPARK-30886 Deprecate two-parameter TRIM/LTRIM/RTRIM") {
+ Seq("trim", "ltrim", "rtrim").foreach { f =>
+ val logAppender = new LogAppender("deprecated two-parameter TRIM/LTRIM/RTRIM functions")
+ def check(count: Int, message: String): Unit = {
Review comment:
the `message` is always the same, do we need this parameter?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595590720
Thank you for review, @cloud-fan , @maropu , and @MaxGekk .
The last commit is only a change on test case. I verified that locally.
Merged to master/3.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595475642
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119407/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381836044
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Is this a suitable place for the warning? How about putting this in `CheckAnalysis` (this is not an error issue though)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595377014
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24147/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588734857
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23454/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344533
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24144/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388256465
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
Seems like one-parameter `ltrim` and `rtrim` have well defined semantic and are consistent with other databases. I'm OK to keep them, although users can do the same thing with the TRIM syntax.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589003698
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118703/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388402537
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
Ur, did you change your mind? This was due to your comment.
- https://github.com/apache/spark/pull/27643#discussion_r381896256
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595377002
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588734857
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23454/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381844272
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Ah, I see.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595206828
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119374/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588724844
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118702/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388438024
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
That was the reason I took a look this for a while. :)
Got it. I narrowed down the scope back.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344056
**[Test build #119407 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119407/testReport)** for PR 27643 at commit [`1b497af`](https://github.com/apache/spark/commit/1b497afbdb165e39f89426deb2a39935fbb0ae61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344528
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595492940
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589003698
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118703/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#discussion_r388471052
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1841,18 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: StringTrim if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM function signature is deprecated." +
Review comment:
My bad. I had to ask from the beginning to avoid my misunderstanding here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588710457
**[Test build #118702 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118702/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381843325
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Oops. I was late. Yes. @cloud-fan 's comment is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588750339
Got it. I'll try tomorrow. Do we have another test example for `log once per session` validation?
> shall we add a test using LogAppender? The expectation is log once per session.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588757429
no AFAIK. But we have many logging tests. You can take a look at the caller side of `LogAppender`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595377002
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r382134079
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1793,6 +1794,7 @@ class Analyzer(
* Replaces [[UnresolvedFunction]]s with concrete [[Expression]]s.
*/
object ResolveFunctions extends Rule[LogicalPlan] {
+ val trimWarningEnabled = new AtomicBoolean(true)
Review comment:
This is an inner object of `Analyzer`, and we will have a new instance for a new session which creates an `Analyzer`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn
two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589002656
**[Test build #118703 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118703/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595492947
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119410/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588734827
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381842297
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Hi, @maropu . Since `CheckAnalysis` received already resolved expressions, there is no way to distinguish `SQL syntax TRIM(trimStr FROM str)` and `function invocation TRIM(trimStr, str)`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588712461
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588732736
**[Test build #118703 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118703/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344533
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24144/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595475637
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588724844
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118702/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun edited a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun edited a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595083970
The PR(code/title/description) is updated.
- Deprecate LTRIM and RTRIM completely.
- Deprecate two-parameter TRIM.
- Add a UT to check one-time warning.
Sorry for being late update.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595080199
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24109/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595206819
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589003685
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595492940
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595206828
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119374/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r382139590
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
Review comment:
Oops. Sure. Let me think about that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588710588
cc @cloud-fan and @gatorsmile
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27643:
[WIP][SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27643: [WIP][SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589204731
Currently, I'm revising this PR for test cases and thinking about LTRIM/RTRIM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588712461
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588724817
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#issuecomment-595344528
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595080199
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24109/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn
two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588710457
**[Test build #118702 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118702/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381842297
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Hi, @maropu . Since `CheckAnalysis` received already resolved expressions, there is no way to distinguish `SQL syntax TRIM(trimStr FROM str)` from Parser and `function invocation TRIM(trimStr, str)` from here.
Also, `failAnalysis("DISTINCT or FILTER specified` also happens here instead of `CheckAnalysis`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588712511
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23453/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595206094
**[Test build #119374 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119374/testReport)** for PR 27643 at commit [`e22b26e`](https://github.com/apache/spark/commit/e22b26ebdba19b45079fda80f29696ce6e6708d6).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388402537
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
Ur, did you change your mind? This was due to your comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595083120
**[Test build #119374 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119374/testReport)** for PR 27643 at commit [`e22b26e`](https://github.com/apache/spark/commit/e22b26ebdba19b45079fda80f29696ce6e6708d6).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595376297
**[Test build #119410 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119410/testReport)** for PR 27643 at commit [`0d87d08`](https://github.com/apache/spark/commit/0d87d0836e84cd482da2df34a29870069a4faabb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588734827
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595376297
**[Test build #119410 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119410/testReport)** for PR 27643 at commit [`0d87d08`](https://github.com/apache/spark/commit/0d87d0836e84cd482da2df34a29870069a4faabb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#discussion_r388465543
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1841,18 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: StringTrim if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM function signature is deprecated." +
Review comment:
Got it. BTW, that will be the same with my first commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595080192
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn
two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588732736
**[Test build #118703 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118703/testReport)** for PR 27643 at commit [`442d5da`](https://github.com/apache/spark/commit/442d5daddf3cb24bec728ae0cf862676cd1e38e9).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595083970
The PR is updated.
- Deprecate LTRIM and RTRIM completely.
- Deprecate two-parameter TRIM.
- Add a UT to check one-time warning.
Sorry for being late update.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388404969
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
Yea I just realized that ltrim/rtrim also has a one-parameter version, and should be kept like the one-parameter trim func.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595475642
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119407/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595080192
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381896256
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
Review comment:
TRIM is an operator which can do the work of LTRIM and RTRIM functions as well, e.g. `TRIM(LEADING trimStr FROM srcStr)` is the same as LTRIM.
There is no FROM syntax for LTRIM and RTRIM as they are just functions. Shall we deprecate LTRIM and RTRIM completely?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595475637
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388702995
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
##########
@@ -768,4 +769,54 @@ class AnalysisSuite extends AnalysisTest with Matchers {
assert(message.startsWith(s"Max iterations ($maxIterations) reached for batch Resolution, " +
s"please set '${SQLConf.ANALYZER_MAX_ITERATIONS.key}' to a larger value."))
}
+
+ test("SPARK-30886 Deprecate two-parameter TRIM/LTRIM/RTRIM") {
+ Seq("trim", "ltrim", "rtrim").foreach { f =>
+ val logAppender = new LogAppender("deprecated two-parameter TRIM/LTRIM/RTRIM functions")
+ def check(count: Int, message: String): Unit = {
Review comment:
You're right. Let me fix that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381840528
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
If we put it in `CheckAnalysis`, we can't distinguish `trim(a, b)` and `trim(a from b)`. They are both `String2TrimExpression`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27643: [SPARK-30886][SQL]
Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595492192
**[Test build #119410 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119410/testReport)** for PR 27643 at commit [`0d87d08`](https://github.com/apache/spark/commit/0d87d0836e84cd482da2df34a29870069a4faabb).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM function
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM function
URL: https://github.com/apache/spark/pull/27643#discussion_r388440051
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1841,18 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: StringTrim if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM function signature is deprecated." +
Review comment:
shall we list all of them? `TRIM` -> `TRIM/LTRIM/RTRIM`, `TRIM(trimStr FROM str)` -> `TRIM((BOTH | LEADING | TRAILING)? trimStr FROM str)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381896256
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
Review comment:
TRIM is an operator which can do the work of LTRIM and RTRIM functions as well, e.g. `TRIM(LEADING trimStr FROM srcStr)`.
There is no FROM syntax for LTRIM and RTRIM as they are just functions. Shall we deprecate LTRIM and RTRIM completely?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-589003685
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27643: [SPARK-30886][SQL]
Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595206819
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381842297
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Hi, @maropu . Since `CheckAnalysis` received already resolved expressions, there is no way to distinguish `SQL syntax TRIM(trimStr FROM str)` from Parser and `function invocation TRIM(trimStr, str)` from here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595377014
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/24147/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27643: [SPARK-30886][SQL]
Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588739784
shall we add a test using `LogAppender`? The expectation is log once per session.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on issue #27643: [SPARK-30886][SQL] Warn
two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
maropu commented on issue #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#issuecomment-588729135
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27643:
[SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27643: [SPARK-30886][SQL] Deprecate two-parameter TRIM/LTRIM/RTRIM functions
URL: https://github.com/apache/spark/pull/27643#issuecomment-595492947
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/119410/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27643:
[SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27643: [SPARK-30886][SQL] Warn two-parameter TRIM/LTRIM/RTRIM function usages
URL: https://github.com/apache/spark/pull/27643#discussion_r381844272
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1837,13 +1839,19 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e: String2TrimExpression if arguments.size == 2 =>
+ if (trimWarningEnabled.get) {
+ log.warn("Two-parameter TRIM/LTRIM/RTRIM function signatures are deprecated." +
+ s" Use SQL syntax `${e.prettyName.toUpperCase(Locale.ROOT)}(trimStr" +
+ s" FROM str)` instead of `${e.prettyName}`. Please refer SPARK-28093.")
+ trimWarningEnabled.set(false)
Review comment:
Ah, I see. Thanks for the explanation, guys!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27643:
[SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27643: [SPARK-30886][SQL] Deprecate LTRIM, RTRIM, and two-parameter TRIM functions
URL: https://github.com/apache/spark/pull/27643#discussion_r388254269
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1839,13 +1842,25 @@ class Analyzer(
}
AggregateExpression(agg, Complete, isDistinct, filter)
// This function is not an aggregate function, just return the resolved one.
- case other =>
- if (isDistinct || filter.isDefined) {
- failAnalysis("DISTINCT or FILTER specified, " +
- s"but ${other.prettyName} is not an aggregate function")
- } else {
- other
+ case other if (isDistinct || filter.isDefined) =>
+ failAnalysis("DISTINCT or FILTER specified, " +
+ s"but ${other.prettyName} is not an aggregate function")
+ case e @ (_: StringTrimLeft | _: StringTrimRight) =>
Review comment:
shall we keep the one-parameter `ltrim` and `rtrim`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org