Skip to content
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.

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

[ZEPPELIN-2411] Improve Table #2323

Closed
wants to merge 42 commits into from

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented May 8, 2017

What is this PR for?

Improve Table

  • column filter
  • persist column state: type, order, hide/show, sorting, pinning
  • pagination
  • setting menu to configure table UI
  • group by + aggregation

And all these things are persisted and synchronized among web socket clients

See the screenshot section for more detail.

What type of PR is it?

[Improvement]

Todos

  • - Remove handsontable dependencies
  • - Use npm packaged moment* packages.
  • - Apply ui-grid
  • - Add setting menu
  • - Fix some issues
  • - Persist column type

What is the Jira issue?

ZEPPELIN-2411

How should this be tested?

  1. Build: mvn clean package -DskipTests; ./bin/zeppelin-daemon.sh restart
  2. Open a note and create tables. If you don't have proper paragraphs, use this snippet.
%spark

import org.apache.commons.io.IOUtils
import java.net.URL
import java.nio.charset.Charset

val bankText = sc.parallelize(
    IOUtils.toString(
        new URL("https://s3.amazonaws.com/apache-zeppelin/tutorial/bank/bank.csv"),
        Charset.forName("utf8")).split("\n"))

case class Bank(
    age: Integer, 
    job: String, 
    marital: String, 
    education: String, 
    balance: Integer,
    housing: Boolean,
    loan: Boolean,
    contact: String,
    day: Int,
    month: String,
    duration: Int,
    y: Boolean
)

val bank = bankText.map(s => s.split(";")).filter(s => s(0) != "\"age\"").map(
    s => Bank(s(0).toInt, 
            s(1).replaceAll("\"", ""),
            s(2).replaceAll("\"", ""),
            s(3).replaceAll("\"", ""),
            s(5).replaceAll("\"", "").toInt,
            if (s(6).replaceAll("\"", "") == "yes") true else false,
            if (s(7).replaceAll("\"", "") == "yes") true else false,
            s(8).replaceAll("\"", ""),
            s(9).replaceAll("\"", "").toInt,
            s(10).replaceAll("\"", ""),
            s(11).replaceAll("\"", "").toInt,
            if (s(16).replaceAll("\"", "") == "yes") true else false
        )
).toDF()
bank.registerTempTable("bank")
select age, education, job, balance from bank limit 1000

Screenshots (if appropriate)

Before

image

After: Filter

2411_new_filter

After: Column related features

2411_new_column

After: Pagination

2411_new_pagination

After: Group, Aggregation

2411_new_group_aggr

After: Synchronized

2411_new_sync

Questions:

  • Does the licenses files need update? - YES, updated
  • Is there breaking changes for older versions? - NO
  • Does this needs documentation? - NO

@1ambda 1ambda closed this May 8, 2017
@1ambda 1ambda reopened this May 8, 2017
@1ambda 1ambda force-pushed the ZEPPELIN-2411/prettify-table branch 2 times, most recently from b933651 to a576cff Compare May 8, 2017 21:18
@1ambda 1ambda closed this May 9, 2017
@1ambda 1ambda reopened this May 9, 2017
@Leemoonsoo
Copy link
Member

Tested and overall it's great improvement!

Here's few feedbacks.

  1. Table does not refreshed after height / width of paragraph change.

  2. How about remove 'type' in column table option?
    image
    I guess user will be okay without knowing it.

  3. showColumnFilter
    Following is screenshot with 'showColumnFilter' on. I don't know what this option supposed to do except for adding empty sticky row in the bottom.
    image

  4. pagenation
    defaultPaginationSize size can be only changed after the Table is displayed with defaultPaginationSize applied. So changing default does not mean too much in this case. Isn't it?

availablePaginationSizes - It's minor opinion, I think it is okay with hardcoded value and not make user configurable.

  1. Header bar in the option
    image

Two buttons - 'Save', 'Delete' icon - on right little bit confuses me. It was difficult to guess what those button supposed to do. And figured out

'save' - apply option to the table.
'delete' - restore to default options

While option change applies immediately, how about hide 'Save' button? And change 'delete' icon to something more appropriate? For example "Restore the default values" text would be more intuitive.

@1ambda
Copy link
Member Author

1ambda commented May 10, 2017

Thanks for the review!

  1. Table does not refreshed after height / width of paragraph change.
  • regarding height, it was not before as well.

2411_height

  • in the case of width, Table will adjust it's size when browser width changes.

2411_width1

regarding the width option, it seems that it doesn't call proper event method for resizing. We can fix it in the different PR. (bug)

image

  1. How about remove 'type' in column table option?
  • Removed
  • While adding tooltip for each option

image

  1. showColumnFilter

It's used to display aggregated values. I changed the name to showAggregationFooter which easier to understand than before.

image

  1. pagination
  • Removed pagination related settings.

image

  1. Header bar in the option
  • Removed the save button.
  • Updated icon for reset and added tooltip for it.

image

@guptarajat
Copy link

"all these things are persisted and synchronized among web socket clients"
What does this mean?

@1ambda
Copy link
Member Author

1ambda commented May 11, 2017

@guptarajat Hi, refer the last GIF. "persist" means that all states in a table are saved in the note.json. For example, sorting state will be saved after this PR but it is not in current master.

@Leemoonsoo
Copy link
Member

Each cell in table supposed to print html. For example

%sh echo -e "%table key\tvalue\nsun\t%html <img width=50px src=http://www.freeiconspng.com/uploads/sun-icon-4.jpg></img>"

This PR
image

In master branch
image

@Leemoonsoo
Copy link
Member

And this PR adds background in rows.

Master branch
image

This PR
image

What do you think? Keep appearance the same as before, so we can keep user experience change minimum. Or is adding background color in rows brings more benefit?

@1ambda
Copy link
Member Author

1ambda commented May 14, 2017

@Leemoonsoo

Colorizing (even rows) is cognitively better because we are not confused when investigating closer rows.

Here are more bright versions

2411_alter

2411_white

@Leemoonsoo
Copy link
Member

Bright version looks better for me

@1ambda 1ambda force-pushed the ZEPPELIN-2411/prettify-table branch from 316f872 to c264cb8 Compare May 15, 2017 01:25
@1ambda
Copy link
Member Author

1ambda commented May 15, 2017

@Leemoonsoo

Oops. I missed that feature.

  1. Now table renders HTML display in the cells and adjusts row height based on the content.

image

  1. Fixed to have more bright even rows.

image

@1ambda 1ambda force-pushed the ZEPPELIN-2411/prettify-table branch from c264cb8 to 2047c5f Compare May 18, 2017 15:42
@1ambda
Copy link
Member Author

1ambda commented May 19, 2017

@Leemoonsoo just rebased and

updated to use white color in each cell background in c56edca
now it looks like

image

@Leemoonsoo
Copy link
Member

GTM. Thanks @1ambda for great improvement.

Merge to master if no further discussions.

// create, compile and append grid elem
gridElem = angular.element(
`<div id="${gridElemId}" ui-grid="${gridElemId}"
ui-grid-edit ui-grid-row-edit
Copy link

@vglushak vglushak Nov 20, 2017

Choose a reason for hiding this comment

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

This made all cells in the table editable. Is that expected?

@vglushak
Copy link

vglushak commented Nov 20, 2017

Now table renders HTML display in the cells and adjusts row height based on the content.

There are some limitations for this feature. img, links works. But when I want to place button it just ignore html and display text only.
Another issue - you can't actually click a link - it starts edit this cell (refer to my previous comment).

Btw, all this works correctly in 0.7.3.

@Tagar
Copy link
Contributor

Tagar commented Feb 21, 2018

Great improvement.
Submitted https://issues.apache.org/jira/browse/ZEPPELIN-3251 to consider showing rows/columns lazily on scrolling events as otherwise ui-grid has a significant overhead for wide datasets so it randers much slower too. Other ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants