Skip to content

[SPARK-37906][SQL] spark-sql should not pass last comment to backend - #35206

Closed
AngersZhuuuu wants to merge 11 commits into
apache:masterfrom
AngersZhuuuu:SPARK-37906
Closed

[SPARK-37906][SQL] spark-sql should not pass last comment to backend#35206
AngersZhuuuu wants to merge 11 commits into
apache:masterfrom
AngersZhuuuu:SPARK-37906

Conversation

@AngersZhuuuu

@AngersZhuuuuAngersZhuuuu commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

In #34815 we change back support unclosed bracketed comment to backend.

But miss the case such as

SELECT 1; --comment
SELECT 1; /* comment */

It's a common use case in sql job. We should ignore the comment at end of SQL script.

Need to clarify that when use -e, we directly pass SQL to splitSemiColon, when use -f, CliDriver will add a \n for query.

 public int processReader(BufferedReader r) throws IOException {
StringBuilder qsb = new StringBuilder();
String line;
while((line = r.readLine()) != null) {
if (!line.startsWith("--")) {
qsb.append(line + "\n");
}
}
return this.processLine(qsb.toString());
}

So in splitSemiColon, we should consider both case.

In this pr, the final behavior like below

For -e

QueryBehavior beforeBehavior now
SELECT 1; --commentWill pass both SELECT 1 and --comment to backend engine and throw exception since --comment can't be executedOnly pass SELECT 1 to backend engine and will ignore the simple comment
SELECT 1; /* comment */ Will pass both SELECT 1 and /* comment */ to backend engine and throw exception since /* comment */ can't be executedOnly pass SELECT 1 to backend engine
SELECT 1; /* comment Will pass SELECT 1 and /* comment to backend engineWill pass SELECT 1 and /* comment to backend engine
SELECT 1; /* comment SELECT 1Will pass SELECT 1 and /* comment SELECT 1 to backend engineWill pass SELECT 1 and /* comment SELECT 1 to backend engine
/ * comment SELECT 1;Will pass / * comment SELECT 1; to back end engine and throw unclose bracketed comment exceptionWill pass / * comment SELECT 1; to back end engine and throw unclose bracketed comment exception

For -f, since -f will add a \n at the end line if it's not started as --

QueryBehavior beforeBehavior now
SELECT 1; --comment\nWill pass both SELECT 1 and --comment to backend engine and throw exception since --comment can't be executedOnly pass SELECT 1 to backend engine and will ignore the simple comment
SELECT 1; /* comment */ \nWill pass both SELECT 1 and /* comment */ to backend engine and throw exception since /* comment */ can't be executedOnly pass SELECT 1 to backend engine
SELECT 1; /* comment \nWill pass SELECT 1 and /* comment\n to backend engineWill pass SELECT 1 and /* comment\n to backend engine
SELECT 1; /* comment SELECT 1\nWill pass SELECT 1 and /* comment SELECT 1\n to backend engineWill pass SELECT 1 and /* comment SELECT 1\n to backend engine
/ * comment SELECT 1;\nWill pass / * comment SELECT 1;\n to back end engine and throw unclose bracketed comment exceptionWill pass / * comment SELECT 1;\n to back end engine and throw unclose bracketed comment exception

Why are the changes needed?

Spark sql should not pass last entire comment to backend

Does this PR introduce any user-facing change?

Use can write SQL script end with a comment

SELECT 1; --comment
SELECT 1; /* comment */

How was this patch tested?

Added UT

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

ping @cloud-fan

@cloud-fan

Copy link
Copy Markdown
Contributor

is it possible to add a test?

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

is it possible to add a test?

Added

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

How about current?

…/thriftserver/SparkSQLCLIDriver.scala
Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
@cloud-fan

Copy link
Copy Markdown
Contributor

thanks, merging to master!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@AngersZhuuuu@cloud-fan