Skip to content

[SPARK-43064][SQL] Spark SQL CLI SQL tab should only show once statement once#40701

Closed
AngersZhuuuu wants to merge 4 commits intoapache:masterfrom
AngersZhuuuu:SPARK-43064
Closed

[SPARK-43064][SQL] Spark SQL CLI SQL tab should only show once statement once#40701
AngersZhuuuu wants to merge 4 commits intoapache:masterfrom
AngersZhuuuu:SPARK-43064

Conversation

@AngersZhuuuu
Copy link
Contributor

What changes were proposed in this pull request?

Before
截屏2023-04-07 下午4 22 54

After
截屏2023-04-07 下午4 24 02

Why are the changes needed?

Don't need show twice, too weird

Does this PR introduce any user-facing change?

No

How was this patch tested?

MT

@github-actions github-actions bot added the SQL label Apr 7, 2023
@AngersZhuuuu
Copy link
Contributor Author

ping @cloud-fan

@cloud-fan
Copy link
Contributor

#40437 might be related. We want to remove hiveResultString from CLI and only use it in hive compatibility tests.

@AngersZhuuuu
Copy link
Contributor Author

ping @cloud-fan

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

cc @sunchao because he is also an expert in this domain.

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

Looks OK to me. It'd be better if we can update the PR description to explain more why this PR is needed and what changes are proposed here.

val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
hiveResponse = SQLExecution.withNewExecutionId(execution, Some("cli")) {
hiveResultString(execution.executedPlan)
// Command type sql have been executed when call `context.sql(command).logicalPlan`,
Copy link
Member

Choose a reason for hiding this comment

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

this comment is not very easy to understand: what is `Command type sql"? maybe:

The SQL command has been executed above via `executePlan`, therefore we don't need to wrap it again
with a new execution ID when getting Hive result

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sunchao sunchao closed this in 4708adc Apr 14, 2023
@sunchao
Copy link
Member

sunchao commented Apr 14, 2023

Merged to master, thanks!

@dongjoon-hyun
Copy link
Member

Thank you all!

Sign up for free to 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.

5 participants