Skip to content

[SPARK-41109][SQL] Rename the error class _LEGACY_ERROR_TEMP_1216 to INVALID_LIKE_PATTERN #38615

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
panbingkun wants to merge 3 commits into from
Closed

[SPARK-41109][SQL] Rename the error class _LEGACY_ERROR_TEMP_1216 to INVALID_LIKE_PATTERN #38615

panbingkun wants to merge 3 commits into from

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Nov 11, 2022

What changes were proposed in this pull request?

In the PR, I propose to rename the legacy error class _LEGACY_ERROR_TEMP_1216 to INVALID_LIKE_PATTERN.

Why are the changes needed?

Proper names of error classes should improve user experience with Spark SQL.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

By running the affected test suites:

$ build/sbt "sql/testOnly *SQLQuerySuite"
$ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z strings.sql"

@panbingkun
Copy link
Contributor Author

panbingkun commented Nov 11, 2022

cc @MaxGekk @itholic

MaxGekk
MaxGekk reviewed Nov 11, 2022
View reviewed changes
core/src/main/resources/error/error-classes.json Outdated
},
"INVALID_LIKE_PATTERN" : {
"message" : [
"The pattern '<pattern>' is invalid, <message>."
Copy link
Member

@MaxGekk MaxGekk Nov 11, 2022

Choose a reason for hiding this comment

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

It would be nice to avoid the message but it is useful in this case. @cloud-fan @srielau WDYT?

Copy link
Contributor

@srielau srielau Nov 11, 2022

Choose a reason for hiding this comment

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

Is it always the same message? If so we could map it.

Copy link
Member

@MaxGekk MaxGekk Nov 12, 2022

Choose a reason for hiding this comment

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

Actually, the message comes from Spark's code, see:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

Line 50 in 70ec696

def fail(message: String) = throw QueryCompilationErrors.invalidPatternError(pattern, message)

and there are just 2 messages:

  1. spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

    Line 59 in 70ec696

    case _ => fail(s"the escape character is not allowed to precede '$c'")

  2. spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

    Line 61 in 70ec696

    case c if c == escapeChar => fail("it is not allowed to end with the escape character")

@panbingkun Could you place the messages to the JSON files instead of leaving them in the source code, please.

Copy link
Contributor Author

@panbingkun panbingkun Nov 13, 2022

Choose a reason for hiding this comment

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

Done

srielau
srielau approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@srielau srielau left a comment

Choose a reason for hiding this comment

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

A comment on mapping the message.

core/src/main/resources/error/error-classes.json Outdated
},
"INVALID_LIKE_PATTERN" : {
"message" : [
"The pattern '<pattern>' is invalid, <message>."
Copy link
Contributor

@srielau srielau Nov 11, 2022

Choose a reason for hiding this comment

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

Is it always the same message? If so we could map it.

@AmplabJenkins
Copy link

AmplabJenkins commented Nov 11, 2022

Can one of the admins verify this patch?

@panbingkun panbingkun requested a review from MaxGekk November 14, 2022 05:31
@MaxGekk
Copy link
Member

MaxGekk commented Nov 14, 2022

+1, LGTM. Merging to master.
Thank you, @panbingkun and @srielau for review.

@MaxGekk MaxGekk closed this in 46246c9 Nov 14, 2022
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 14, 2022

Hi, @MaxGekk . It seems that this PR didn't passed CI properly. Could you check once more?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 14, 2022

I made a PR.

dongjoon-hyun
dongjoon-hyun reviewed Nov 14, 2022
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

import org.apache.spark.{AccumulatorSuite, SparkException}
import org.apache.spark.scheduler.{SparkListener, SparkListenerJobStart}
import org.apache.spark.sql.catalyst.expressions.Cast._
Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 14, 2022

Choose a reason for hiding this comment

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

This addition was the root cause of Scala style check failure.

LuciferYang
LuciferYang reviewed Nov 14, 2022
View reviewed changes
core/src/main/resources/error/error-classes.json
"The pattern <pattern> is invalid."
],
"subClass" : {
"ESC_IN_THE_MIDDLE" : {
Copy link
Contributor

@LuciferYang LuciferYang Nov 14, 2022

Choose a reason for hiding this comment

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

sub class order is wrong, should be ESC_AT_THE_END then ESC_IN_THE_MIDDLE

#38658 for fix this

@LuciferYang LuciferYang mentioned this pull request Nov 14, 2022
Closed
@panbingkun panbingkun deleted the error_classes_invalid_like_pattern branch November 19, 2022 00:58
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
@panbingkun @SandishKumarHN
[SPARK-41109][SQL] Rename the error class _LEGACY_ERROR_TEMP_1216 to …
08b08e0 
…INVALID_LIKE_PATTERN

### What changes were proposed in this pull request?
In the PR, I propose to rename the legacy error class _LEGACY_ERROR_TEMP_1216 to INVALID_LIKE_PATTERN.

### Why are the changes needed?
Proper names of error classes should improve user experience with Spark SQL.

### Does this PR introduce _any_ user-facing change?
Yes.

### How was this patch tested?
By running the affected test suites:
> $ build/sbt "sql/testOnly *SQLQuerySuite"
> $ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z strings.sql"

Closes apache#38615 from panbingkun/error_classes_invalid_like_pattern.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LuciferYang LuciferYang LuciferYang left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@MaxGekk MaxGekk MaxGekk approved these changes

+1 more reviewer

@srielau srielau srielau approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@panbingkun @AmplabJenkins @MaxGekk @dongjoon-hyun @LuciferYang @srielau