Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Apr 27, 2020

What changes were proposed in this pull request?

This PR adds a font color setting of DAG-viz for query plan.

Why are the changes needed?

#28352 aimed to unify the font color of all DAG-viz in WebUI but there is one part left over.

Before this change applied, the appearance of a query plan is like as follows.
plan-graph-fixed
The color of WholeStageCodegen (1) and its following text (duration: total...) is slightly darker than SerializeFromObject.

After this change, those color is unified as #333333.
plan-graph-fixed2

Does this PR introduce any user-facing change?

Slightly yes.

How was this patch tested?

I confirmed the style of fill and color is #333333 by debug console of Chrome.
fill
color

@SparkQA
Copy link

SparkQA commented Apr 27, 2020

Test build #121860 has finished for PR 28355 at commit 6982a17.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

font-weight: bold;
}

#plan-viz-graph svg text {
Copy link
Member

Choose a reason for hiding this comment

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

Previously, we handled all .label. Now, this PR is about text.
If we start to consider text, are you sure that this is the last text?

Copy link
Member Author

@sarutak sarutak Apr 27, 2020

Choose a reason for hiding this comment

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

I think we need to consider three part of DAG-viz. Stages, RDDs and query plans.

In the previous PR, I changed the color of .label for stages and RDDs. Both are covered here.
I also changed color of .label for query plans here.

About text, for stages and RDDs, it's already covered here.
So I think the last one is for query plans. It's covered by this PR.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Apr 27, 2020

Choose a reason for hiding this comment

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

Thanks for the confirmation! Now, it's clear to me.

@dongjoon-hyun
Copy link
Member

Retest this please.

@dongjoon-hyun
Copy link
Member

@sarutak . I checked the WholeStageCodegen (1) is changed from #000000 to #333333 correctly. In the same way, the text below WholeStageCodegen (1) like the followings are also changed together. If this is intentional, please update the PR description accordingly.

duration: total (min, med, max )
545 ms (17 ms, 53 ms, 113 ms )

@sarutak
Copy link
Member Author

sarutak commented Apr 27, 2020

@dongjoon-hyun Thanks for checking. The appearance you mention is intentional so I've updated the description.

@SparkQA
Copy link

SparkQA commented Apr 27, 2020

Test build #121867 has finished for PR 28355 at commit 6982a17.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Apr 27, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented Apr 27, 2020

Test build #121908 has finished for PR 28355 at commit 6982a17.

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

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.

+1, LGTM. Thank you, @sarutak .
Merged to master/3.0.

dongjoon-hyun pushed a commit that referenced this pull request Apr 27, 2020
…query plan

### What changes were proposed in this pull request?

This PR adds a font color setting of DAG-viz for query plan.

### Why are the changes needed?

#28352 aimed to unify the font color of all DAG-viz in WebUI but there is one part left over.

Before this change applied, the appearance of a query plan is like as follows.
<img width="456" alt="plan-graph-fixed" src="https://user-images.githubusercontent.com/4736016/80325600-ca4d4e00-8870-11ea-945c-64971dbb752c.png">
The color of `WholeStageCodegen (1)` and its following text (`duration: total...`) is slightly darker than `SerializeFromObject`.

After this change, those color is unified as `#333333`.
<img width="450" alt="plan-graph-fixed2" src="https://user-images.githubusercontent.com/4736016/80325651-fb2d8300-8870-11ea-8ed8-178c124d224c.png">

### Does this PR introduce any user-facing change?

Slightly yes.

### How was this patch tested?

I confirmed the style of `fill` and `color` is `#333333` by debug console of Chrome.
<img width="321" alt="fill" src="https://user-images.githubusercontent.com/4736016/80325760-6c6d3600-8871-11ea-82e7-e789bf741f2a.png">
<img width="316" alt="color" src="https://user-images.githubusercontent.com/4736016/80325765-70995380-8871-11ea-8976-7020205d585c.png">

Closes #28355 from sarutak/followup-SPARK-31565.

Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 7d4d05c)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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.

3 participants