Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Sep 22, 2019

What changes were proposed in this pull request?

This PR add support sorting Execution Time and Duration columns for ThriftServerSessionPage.

Why are the changes needed?

Previously, it's not sorted correctly.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

Manually do the following and test sorting on those columns in the Spark Thrift Server Session Page.

$ sbin/start-thriftserver.sh
$ bin/beeline -u jdbc:hive2://localhost:10000
0: jdbc:hive2://localhost:10000> create table t(a int);
+---------+--+
| Result  |
+---------+--+
+---------+--+
No rows selected (0.521 seconds)
0: jdbc:hive2://localhost:10000> select * from t;
+----+--+
| a  |
+----+--+
+----+--+
No rows selected (0.772 seconds)
0: jdbc:hive2://localhost:10000> show databases;
+---------------+--+
| databaseName  |
+---------------+--+
| default       |
+---------------+--+
1 row selected (0.249 seconds)

Sorted by Execution Time column:
image

Sorted by Duration column:
image

@wangyum
Copy link
Member Author

wangyum commented Sep 22, 2019

cc @amanomer

@amanomer
Copy link
Contributor

Looks good. Nice catch @wangyum .

@SparkQA
Copy link

SparkQA commented Sep 22, 2019

Test build #111154 has finished for PR 25892 at commit e794fe2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@amanomer
Copy link
Contributor

amanomer commented Sep 22, 2019

I would like to take this in branch-2.4 by #25882.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 22, 2019

@amanomer and @wangyum .
It seems that idea is already reverted from #25882.

In general, please do not mix different ones. Especially, it's not a good idea when the original one is not merged yet. :)

@dongjoon-hyun
Copy link
Member

BTW, @wangyum .
As a committer, please make it sure if the PR keeps the original four sections template of the PR.
For me, it seems that it's removed accidentally in this PR. It will be better if you recover it. 😄
cc @HyukjinKwon .

@amanomer
Copy link
Contributor

Ok. I have created another PR #25893 to back-port this to branch-2.4.

@dongjoon-hyun
Copy link
Member

Hi, @wangyum .
Did you check the original STS behavior?

@dongjoon-hyun
Copy link
Member

In addition, this PR description is wrong. This is UI PR and that's the reason why you attach the screenshot. Could you fix the PR description?

Does this PR introduce any user-facing change?

No.

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.

Note that I fixed the PR description issues by

  1. Recovering the missing section
  2. Fixing UI section from NO to YES.
  3. Updating the test procedure for reviewers. (The section is for the reviewers mainly.)

+1, LGTM. Thank you, @wangyum and @amanomer .

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-28599][SQL] Support sorting Execution Time and Duration columns for ThriftServerSessionPage [SPARK-28599][SQL] Fix Execution Time and Duration column sorting for ThriftServerSessionPage Sep 22, 2019
@dongjoon-hyun
Copy link
Member

Merged to master.

@dongjoon-hyun
Copy link
Member

I changed SPARK-28599 from Improvement to Bug.
Since this is a bug fix, we can backport this merged commit (with the fixed commit messages). However, in branch-2.4, there is no Execution Time column. There is Duration column only.
Please make it sure during backporting.

@wangyum wangyum deleted the SPARK-28599 branch September 22, 2019 23:18
amanomer pushed a commit to amanomer/spark that referenced this pull request Sep 23, 2019
… for ThriftServerSessionPage

This PR add support sorting `Execution Time` and `Duration` columns for `ThriftServerSessionPage`.

Previously, it's not sorted correctly.

Yes.

Manually do the following and test sorting on those columns in the Spark Thrift Server Session Page.
```
$ sbin/start-thriftserver.sh
$ bin/beeline -u jdbc:hive2://localhost:10000
0: jdbc:hive2://localhost:10000> create table t(a int);
+---------+--+
| Result  |
+---------+--+
+---------+--+
No rows selected (0.521 seconds)
0: jdbc:hive2://localhost:10000> select * from t;
+----+--+
| a  |
+----+--+
+----+--+
No rows selected (0.772 seconds)
0: jdbc:hive2://localhost:10000> show databases;
+---------------+--+
| databaseName  |
+---------------+--+
| default       |
+---------------+--+
1 row selected (0.249 seconds)
```

**Sorted by `Execution Time` column**:
![image](https://user-images.githubusercontent.com/5399861/65387476-53038900-dd7a-11e9-885c-fca80287f550.png)

**Sorted by `Duration` column**:
![image](https://user-images.githubusercontent.com/5399861/65387481-6e6e9400-dd7a-11e9-9318-f917247efaa8.png)

Closes apache#25892 from wangyum/SPARK-28599.

Authored-by: Yuming Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
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.

4 participants