Skip to content

Conversation

@ankurmitujjain
Copy link

@ankurmitujjain ankurmitujjain commented Mar 4, 2016

What is this PR for?

It is an extension of #6 #714
It allows user to export data in a paragraph to a TSV/CSV file.

What type of PR is it?

Feature

Todos

  • - Improves the current Table features like Search, Fixed Headers, Sorting

Is there a relevant Jira issue?

ZEPPELIN-672

How should this be tested?

  1. Create a paragraph with data in %table view
  2. Click on TSV/CSV button to export CSV/TSV file

Screenshots (if appropriate)

image

Questions:

  • Does the licenses files need update?
    Need to have MIT license for Datatables.
  • Is there breaking changes for older versions?
    No
  • Does this needs documentation?
    No

@ankurmitujjain ankurmitujjain changed the title Add feature to export tables using Datatables [ZEPPELIN-672] Add feature to export tables using Datatables Mar 4, 2016
@ankurmitujjain
Copy link
Author

@adejanovski @corneadoug @bzz @Leemoonsoo
I am done with integrating datatables, have to update a failing testcase...

@corneadoug can you please let me know any example for Chart Pivoted data to verify if this is completed or not...

Thanks
Ankur

@ankurmitujjain
Copy link
Author

@adejanovski @corneadoug @bzz @Leemoonsoo @felixcheung

Hello Team,

Can you please review this PR?

Thanks
Ankur

@corneadoug
Copy link
Contributor

Thanks, I'll test that out

@prabhjyotsingh
Copy link
Contributor

I see following issues:

  • There is a horizontal scrollbar
  • When i scroll, table data goes above header

screen shot 2016-03-09 at 4 22 58 pm

@ankurmitujjain
Copy link
Author

Thanks for reviewing @prabhjyotsingh
-I will look into horizontal scroll bar

Thanks
Ankur

--There is a horizontal scrollbar
--When i scroll, table data goes above header

Signed-off-by: ankur_jain <[email protected]>
@ankurmitujjain
Copy link
Author

Hi @prabhjyotsingh ,

I had updated code as per your comments please review..

Thanks
Ankur

@prabhjyotsingh
Copy link
Contributor

Tested it with different size of data. Looks good to me.

@ankurmitujjain
Copy link
Author

Hi @corneadoug can we merge it ?

@felixcheung
Copy link
Member

@hyonaldo @zhongneu you have both created PRs on this before, are you ok with this approach?

@corneadoug
Copy link
Contributor

I found some css problems depending on the paragraph size:
screen shot 2016-03-16 at 2 13 51 pm

Also, when we scroll, the download buttons and search input can't be seen:
screen shot 2016-03-16 at 2 14 10 pm

Btw, I saw that the Allowing Export to be not only Table based, but also of Chart Pivoted data task is checked, however I didn't see any button when switching to a graph

@ankurmitujjain
Copy link
Author

Hi @corneadoug : Sorry I wrongly marked pivot chart data as complete...
Actually I have to remove that feature for now, I will be exploring on how we can export data from graphs as per you suggestion, but for now I would like to keep this feature away from this PR...

However we can always export graph data in table view.

This PR I had planned only for implementing datatable and its export feature....
Please let me know if you are ok with this...

Regarding CSS problem: I will look into it...

Thanks
Ankur

@jmrr
Copy link

jmrr commented Apr 5, 2016

@ankurjainyash any progress with this? I'm experiencing some merge conflicts and master is evolving fast.

@Leemoonsoo I've seen that the export capability is clearly set in the roadmap you sent few weeks ago. What do you think it will be the final approach? Any of #725 , #714 , #6 #89 or #672 or a combination of some/all? I've tested #672 and its data display with search/sort seems more complete.

@corneadoug
Copy link
Contributor

I think we would be better off using multiple PRs:

The good thing would be that both back-end and front-end data download would use the same UI, and it wouldn't add additional UI elements in the view

@r-kamath
Copy link
Member

r-kamath commented Apr 5, 2016

#761 to replace the table only (if it brings better performances)

What is the max number of rows this lib can support ?

Conflicts:
	zeppelin-web/src/app/notebook/paragraph/paragraph.css
This reverts commit adb66a3, reversing
changes made to 6363e97.

Conflicts:
	conf/zeppelin-site.xml.template
	zeppelin-zengine/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
Signed-off-by: ankur_jain <[email protected]>
Conflicts:
	docs/interpreter/R.md
	zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js
	zeppelin-web/src/app/notebook/paragraph/paragraph.css
@ankurmitujjain
Copy link
Author

@corneadoug , please also review comments from other users... #6.

@fireboy1919
Copy link
Contributor

See: #822

@corneadoug
Copy link
Contributor

@ankurmitujjain I made a PR to your branch ankurmitujjain#1

@ankurmitujjain
Copy link
Author

@fireboy1919 is it possible to download output in the form of CSV/TSV?

@fireboy1919
Copy link
Contributor

Yes, #822 does that. It's purely backend, using resource pools. You might consider it an alternative to using #714.

@ankurmitujjain
Copy link
Author

ok if thats the case than I believe datatable dont need download functionality which will be handled by #822 . please correct me if I am wrong...

@ankurmitujjain
Copy link
Author

@fireboy1919 is it possible to download both server side data and front end table data?

@fireboy1919
Copy link
Contributor

It's a REST call, so potentially, you could make a client side call to combine the two pieces of information.

But the problem is the same one the backend is solving by being backend - the memory limits imposed by the browser are small compared to what the server can handle.

A smarter approach would probably be to get that kind of thing would be to make a different kind backend component that can transform the data the way that you want it to be transformed.

@ankurmitujjain
Copy link
Author

So basically you are suggesting to use rest endpoints in download buttons to download server side data....

@darionyaphet
Copy link

darionyaphet commented Apr 15, 2016

hi @ankurjainyash could support a option about save the CSV/TSV to server ? It's very useful .

@ankurmitujjain
Copy link
Author

@darionyaphet . If you want to save data as csv on server you can use below code...

spark-csv

 val selectedData = df.select("year", "model")
 selectedData.write
.format("com.databricks.spark.csv")
.option("header", "true")
.save("newcars.csv")

@darionyaphet
Copy link

@ankurmitujjain 👍

Updated testcases for @corneadoug pull request
@johnnyws johnnyws mentioned this pull request Apr 19, 2016
4 tasks
@ankurmitujjain
Copy link
Author

Updated code to fix testcase, please review

@corneadoug
Copy link
Contributor

Merging this

@felixcheung
Copy link
Member

Could you check out #858? Seems to overlap with this PR
#858

@corneadoug
Copy link
Contributor

@felixcheung already saw it and I will check it.
However, while we test + discuss the other PR, there is no reason not to merge this one.
If the other is better, then it can simply replace this one

@asfgit asfgit closed this in d481b56 Apr 30, 2016
asfgit pushed a commit that referenced this pull request May 9, 2016
### What is this PR for?
After #761, on compiling project this file `karma.conf.js` started showing diff.

### What type of PR is it?
[Hot Fix]

### Todos
* [x] - Add karma.conf.js with diff.

### What is the Jira issue?
N/A

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Prabhjyot Singh <[email protected]>

Closes #875 from prabhjyotsingh/datatables_missing_karam_conf and squashes the following commits:

f313878 [Prabhjyot Singh] add missing karma.conf.js
onkarshedge pushed a commit to onkarshedge/incubator-zeppelin that referenced this pull request May 11, 2016
### What is this PR for?
It is an extension of apache#6 apache#714
It allows user to export data in a paragraph to a TSV/CSV file.

### What type of PR is it?
Feature

### Todos
* [x] - Improves the current Table features like Search, Fixed Headers, Sorting

### Is there a relevant Jira issue?
[ZEPPELIN-672](https://issues.apache.org/jira/browse/ZEPPELIN-672)

### How should this be tested?
1. Create a paragraph with data in %table view
2. Click on TSV/CSV button to export CSV/TSV file

### Screenshots (if appropriate)
![image](https://cloud.githubusercontent.com/assets/1140475/13525760/4913bd8e-e229-11e5-9cd5-480c8b583d5b.png)

### Questions:
* Does the licenses files need update?
 Need to have MIT license for Datatables.
* Is there breaking changes for older versions?
No
* Does this needs documentation?
No

Author: ankur_jain <[email protected]>
Author: Ankur Jain <[email protected]>
Author: Damien CORNEAU <[email protected]>

Closes apache#761 from ankurmitujjain/master and squashes the following commits:

4ddcc0f [Ankur Jain] Updated testcases for @corneadoug pull request
e6470aa [Ankur Jain] Merge pull request apache#1 from corneadoug/clean/dataframe
dd8901b [Damien CORNEAU] last fixes
5aca081 [Damien CORNEAU] Last Modifications
9c4412f [Damien CORNEAU] Remove buttons
2561630 [Ankur Jain] Updated for indent
c9b675d [Ankur Jain] Updated for indent
38ee3c3 [Ankur Jain] Updated for indent
b23cab4 [Ankur Jain] Updated for indent
09c87a0 [Ankur Jain] Updated for indent
e4b3abb [ankur_jain] Removed R.md accidentally added
d3aadc6 [ankur_jain] Updated testcase
210b7a6 [ankur_jain] Updates latest code of controller
80bd58c [ankur_jain] Merge branch 'upstream/master'
0ee76b1 [ankur_jain] Update 3 files
0c5f623 [ankur_jain] Revert "Merge branch 'upstream/master'"
adb66a3 [ankur_jain] Merge branch 'upstream/master'
6363e97 [ankur_jain] Merge branch 'master' of https://github.com/ankurmitujjain/incubator-zeppelin
0c94cab [ankur_jain] Merge branch 'master' of https://github.com/ankurmitujjain/incubator-zeppelin
d23202e [ankur_jain] Merge remote-tracking branch 'refs/remotes/origin/master' into apache/master
415c1f5 [ankur_jain] Merge branch 'apache/master'
7901f5e [ankur_jain] Merge branch 'refs/heads/master' into apache/master
6e6587b [ankur_jain] Updating codebase as per @prabhjyotsingh comments
aea8446 [ankur_jain] Merge branch 'apache/master'
df1620c [ankur_jain] Updated testcase as resultant paragraph have text of buttons and search box.
00b36e5 [ankur_jain] Reverted line 117 and 2122 as per previous code
9351a0d [ankur_jain] Committed for Datatables apache#6
onkarshedge pushed a commit to onkarshedge/incubator-zeppelin that referenced this pull request May 11, 2016
### What is this PR for?
After apache#761, on compiling project this file `karma.conf.js` started showing diff.

### What type of PR is it?
[Hot Fix]

### Todos
* [x] - Add karma.conf.js with diff.

### What is the Jira issue?
N/A

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Prabhjyot Singh <[email protected]>

Closes apache#875 from prabhjyotsingh/datatables_missing_karam_conf and squashes the following commits:

f313878 [Prabhjyot Singh] add missing karma.conf.js
@darionyaphet
Copy link

This PR have merge into master branch ? @ankurmitujjain @prabhjyotsingh

@ankurmitujjain ankurmitujjain changed the title [ZEPPELIN-672] Add feature to export tables using Datatables ZEPPELIN-672] Add Datatables instead of normal tables Jun 8, 2016
@AhyoungRyu
Copy link
Contributor

@darionyaphet Yes it does. See here : d481b56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants