Skip to content

[ZEPPELIN-4522]. Support multiple sql statements for SparkSqlInterpreter#3579

Closed
zjffdu wants to merge 4 commits intoapache:masterfrom
zjffdu:ZEPPELIN-4522
Closed

[ZEPPELIN-4522]. Support multiple sql statements for SparkSqlInterpreter#3579
zjffdu wants to merge 4 commits intoapache:masterfrom
zjffdu:ZEPPELIN-4522

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Dec 28, 2019

What is this PR for?

Use the SqlSplitter in zeppelin-interpreter to split sql and execute in SparkSqlInterpreter. Nothing changes for the previous single sql statement paragraph. But just multiple result will be displayed for multiple sql statements.

What type of PR is it?

[Feature]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • CI pass

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Comment on lines 92 to 93
sc.setLocalProperty("spark.scheduler.pool", context.getLocalProperties().get("pool"));
sc.setJobGroup(Utils.buildJobGroupId(context), Utils.buildJobDesc(context), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to move this outside of the loop? Or at least move some things like, Utils.buildJobGroupId(context), Utils.buildJobDesc(context), ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense

sc.setJobGroup(Utils.buildJobGroupId(context), Utils.buildJobDesc(context), false);

try {
Method method = sqlc.getClass().getMethod("sql", String.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this as well?

"" + sparkInterpreter.getZeppelinContext().getMaxResult()));
String result = sparkInterpreter.getZeppelinContext().showData(
method.invoke(sqlc, sql), maxResult);
sc.clearJobGroup();
Copy link
Contributor

Choose a reason for hiding this comment

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

And move this to finally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, and I think we didn't reset the local property(job group is just a local property) in other places, created ZEPPELIN-4523 to track it.

sc.clearJobGroup();
builder.append(result);
} catch (Exception e) {
if (Boolean.parseBoolean(getProperty("zeppelin.spark.sql.stacktrace"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe output the SQL clause that caused error as part of the error, in addition to exception? I don't remember, if Spark SQL always shows the original SQL, or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, fixed it

method.invoke(sqlc, st), maxResult);
sc.clearJobGroup();
return new InterpreterResult(Code.SUCCESS, msg);
for (String sql : sqls) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what will be in the builder if we'll have empty list of SQLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty will be returned if there's no sql, I just added one test case which only 2 sql comments in the paragraph text. Thanks for the review @alexott

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

LGTM, except only question on the empty list of SQLs...

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

LGTM

@zjffdu zjffdu force-pushed the ZEPPELIN-4522 branch 4 times, most recently from fc38622 to 8fee4c3 Compare December 30, 2019 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants