Skip to content

[SPARK-31595][SQL] Spark sql should allow unescaped quote mark in quoted string #28393

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

Closed
adrian-wang wants to merge 4 commits into from
Closed

[SPARK-31595][SQL] Spark sql should allow unescaped quote mark in quoted string #28393

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
562841b
SPARK-31595 Spark sql cli should allow unescaped quote mark in quoted…
adrian-wang Apr 28, 2020
1947cbe
refine comments
adrian-wang May 1, 2020
c20543d
add jira title in test suite
adrian-wang May 2, 2020
d185264
remove multiline string style
adrian-wang May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...hriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@ private[hive] class SparkSQLCLIDriver extends CliDriver with Logging {
}

// Adapted splitSemiColon from Hive 2.3's CliDriver.splitSemiColon.
// Allow "'" and '"' as closing legal string
private def splitSemiColon(line: String): JList[String] = {
var insideSingleQuote = false
var insideDoubleQuote = false
Expand All @@ -519,13 +520,13 @@ private[hive] class SparkSQLCLIDriver extends CliDriver with Logging {
for (index <- 0 until line.length) {
if (line.charAt(index) == '\'' && !insideComment) {
// take a look to see if it is escaped

@dilipbiswal dilipbiswal Apr 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@adrian-wang Should we update the comment to reflect the newly added condition?

@xuanyuanking xuanyuanking Apr 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, maybe we can rephrase this comment here.

if (!escape) {
if (!escape && !insideDoubleQuote) {
// flip the boolean variable
insideSingleQuote = !insideSingleQuote
}
} else if (line.charAt(index) == '\"' && !insideComment) {
// take a look to see if it is escaped

@dilipbiswal dilipbiswal Apr 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@adrian-wang Same.

if (!escape) {
if (!escape && !insideSingleQuote) {
// flip the boolean variable
insideDoubleQuote = !insideDoubleQuote
}
Expand Down
9 changes: 9 additions & 0 deletions sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,4 +500,13 @@ class CliSuite extends SparkFunSuite with BeforeAndAfterAll with BeforeAndAfterE
| ;""".stripMargin -> "testcomment"
)
}

test("Should allow unescaped quote mark in quoted string") {

@maropu maropu May 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Plz add a prefix SPARK-31595: Should allow....

runCliWithin(1.minute)(

@maropu maropu May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explicitly set false at spark.sql.parser.escapedStringLiterals for the tests below?

@adrian-wang adrian-wang May 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually this has nothing to do with spark.sql.parser.escapedStringLiterals, even if the config is set to true, the parser should accept this string.

@maropu maropu May 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ur, I got it. I think the option is misleading... Could you remove it from the PR descritpion?

@adrian-wang adrian-wang May 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done that, thanks!

"""SELECT '"legal string a';select 1 + 234;""".stripMargin -> "235"

@maropu maropu May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this an only issue in the thrift server side? How about the spark side?

scala> sql("""SELECT '"legal string a';select 1 + 234;""").show()
org.apache.spark.sql.catalyst.parser.ParseException:
extraneous input 'select' expecting {<EOF>, ';'}(line 1, pos 25)

== SQL ==
SELECT '"legal string a';select 1 + 234;
-------------------------^^^

  at org.apache.spark.sql.catalyst.parser.ParseException.withCommand(ParseDriver.scala:268)
  at org.apache.spark.sql.catalyst.parser.AbstractSqlParser.parse(ParseDriver.scala:135)
  at org.apache.spark.sql.execution.SparkSqlParser.parse(SparkSqlParser.scala:49)

@adrian-wang adrian-wang May 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not the same case. sql() only accepts single sql statement, even

sql("select 1; select 2;")

will return error.

@maropu maropu May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see.

)
runCliWithin(1.minute)(
"""SELECT "legal 'string b";select 22222 + 1;""".stripMargin -> "22223"

@cloud-fan cloud-fan May 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: let's not use the multiline string style for a single line string.

@adrian-wang adrian-wang May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

)
}
}