Skip to content

[SPARK-31983] Fix Sorting for duration column and make Status column sortable #28752

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

Jump to bottom
Closed
iRakson wants to merge 1 commit into from
Closed

[SPARK-31983] Fix Sorting for duration column and make Status column sortable #28752

iRakson wants to merge 1 commit into from

Conversation

@iRakson
Copy link
Contributor

@iRakson iRakson commented Jun 8, 2020 •
edited
Loading

What changes were proposed in this pull request?

In #28485 pagination support for tables of Structured Streaming Tab was added.
It missed 2 things:

  • For sorting duration column, String was used which sometimes gives wrong results(consider "3 ms" and "12 ms"). Now we first sort the duration column and then convert it to readable String
  • Status column was not made sortable.

Why are the changes needed?

To fix the wrong result for sorting and making Status column sortable.

Does this PR introduce any user-facing change?

No

How was this patch tested?

After changes:
Screenshot 2020-06-08 at 2 18 48 PM

@iRakson
Copy link
Contributor Author

iRakson commented Jun 8, 2020

cc @sarutak @srowen Kindly take a look.

srowen
srowen reviewed Jun 8, 2020
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks good. Are there other similar cases, I wonder?

@iRakson
Copy link
Contributor Author

iRakson commented Jun 8, 2020

Looks good. Are there other similar cases, I wonder?

I will look into other places as well.

@SparkQA
Copy link

SparkQA commented Jun 8, 2020

Test build #123624 has finished for PR 28752 at commit acd9d54.

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

@sarutak
Copy link
Member

sarutak commented Jun 8, 2020

Looks good. Pending for other similar cases.

@iRakson
Copy link
Contributor Author

iRakson commented Jun 8, 2020

I checked. I cannot find any similar case for other tables.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 9, 2020 •
edited
Loading

@gengliangwang too FYI

@iRakson
Copy link
Contributor Author

iRakson commented Jun 10, 2020

gentle ping @srowen @sarutak @HyukjinKwon

@iRakson iRakson requested a review from srowen June 12, 2020 06:44
@srowen
Copy link
Member

srowen commented Jun 12, 2020

LGTM. Do you all have an opinion on back porting to branch-3.0? I think I would.

@sarutak
Copy link
Member

sarutak commented Jun 13, 2020

LGTM. Do you all have an opinion on back porting to branch-3.0? I think I would.
Yeah, it would be better. What do you think @iRakson ?

@iRakson
Copy link
Contributor Author

iRakson commented Jun 13, 2020 •
edited
Loading

LGTM. Do you all have an opinion on back porting to branch-3.0? I think I would.
Yeah, it would be better. What do you think @iRakson ?

Just to be clear, We are talking about back porting pagination support or fixing this sorting issue ?

If we are only fixing this sorting issue then implementation will be slightly different as we are using listingTable for generating table.
Either way it would be good to fix this.

@sarutak
Copy link
Member

sarutak commented Jun 13, 2020

Pagination feature is not in branch-3.0 so how about focusing the sorting issue?

@iRakson
Copy link
Contributor Author

iRakson commented Jun 13, 2020

Ok. I will raise a PR for that. Just one doubt, for fixing in 3.0 should I file a new JIRA(as implementation will be slightly different) or just raise as a follow up for this only.

@iRakson iRakson mentioned this pull request Jun 13, 2020
Closed
@srowen
Copy link
Member

srowen commented Jun 13, 2020

We will merge this just to master. If you can cleanly fix in 3.0 too, yes that's fine in a new PR. It should be the same JIRA.

@iRakson
Copy link
Contributor Author

iRakson commented Jun 13, 2020

We will merge this just to master. If you can cleanly fix in 3.0 too, yes that's fine in a new PR. It should be the same JIRA.

Oh. I created a new JIRA. SPARK-31642 is for introducing pagination support in structured streaming tab, so I filed another JIRA(https://issues.apache.org/jira/browse/SPARK-31983).

I raised a PR here.

@srowen srowen closed this in f5f6eee Jun 14, 2020
@srowen
Copy link
Member

srowen commented Jun 14, 2020

Merged to master. I'll look at the backport.

@srowen
Copy link
Member

srowen commented Jun 14, 2020

Oh I see, this was attached to the JIRA for pagination. It does make sense to have a separate JIRA for that. Let me retroactively fix up the links.

@srowen srowen changed the title [SPARK-31642][FOLLOWUP] Fix Sorting for duration column and make Status column sortable [SPARK-31983] Fix Sorting for duration column and make Status column sortable Jun 14, 2020
sarutak pushed a commit that referenced this pull request Jun 15, 2020
@iRakson @sarutak
[SPARK-31983][WEBUI][3.0] Fix sorting for duration column in structur…
9ca5934 
…ed streaming tab

### What changes were proposed in this pull request?
Sorting result for duration column in tables of structured streaming tab is wrong sometimes.
<img width="1677" alt="Screenshot 2020-06-13 at 1 58 53 PM" src="https://user-images.githubusercontent.com/15366835/84572178-10755700-adb6-11ea-9131-338e8ba7fb24.png">

We are sorting on string, which results in this behaviour.
`sorttable_numeric` and `sorttable_customkey` is used to fix this.

Refer [this](#28752 (comment)) and [this](#28752 (comment))

After changes :
<img width="1677" alt="Screenshot 2020-06-13 at 8 05 32 PM" src="https://user-images.githubusercontent.com/15366835/84572299-a8734080-adb6-11ea-9aa3-b4bc594de4cf.png">

### Why are the changes needed?
Sorting results are wrong for duration column in tables of structured streaming tab.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Screenshots attached.

Closes #28823 from iRakson/testsort.

Authored-by: iRakson <[email protected]>
Signed-off-by: Kousuke Saruta <[email protected]>
@iRakson
Copy link
Contributor Author

iRakson commented Jun 15, 2020

Thank You. @srowen @sarutak.

holdenk pushed a commit to holdenk/spark that referenced this pull request Jun 25, 2020
@iRakson @dongjoon-hyun
[SPARK-31983][WEBUI][3.0] Fix sorting for duration column in structur…
38186b0 
…ed streaming tab

### What changes were proposed in this pull request?
Sorting result for duration column in tables of structured streaming tab is wrong sometimes.
<img width="1677" alt="Screenshot 2020-06-13 at 1 58 53 PM" src="https://user-images.githubusercontent.com/15366835/84572178-10755700-adb6-11ea-9131-338e8ba7fb24.png">

We are sorting on string, which results in this behaviour.
`sorttable_numeric` and `sorttable_customkey` is used to fix this.

Refer [this](apache#28752 (comment)) and [this](apache#28752 (comment))

After changes :
<img width="1677" alt="Screenshot 2020-06-13 at 8 05 32 PM" src="https://user-images.githubusercontent.com/15366835/84572299-a8734080-adb6-11ea-9aa3-b4bc594de4cf.png">

### Why are the changes needed?
Sorting results are wrong for duration column in tables of structured streaming tab.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Screenshots attached.

Closes apache#28823 from iRakson/testsort.

Authored-by: iRakson <[email protected]>
Signed-off-by: Kousuke Saruta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srowen srowen Awaiting requested review from srowen

Assignees

No one assigned

Labels

SQL STRUCTURED STREAMING WEB UI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iRakson @SparkQA @sarutak @HyukjinKwon @srowen