Skip to content

Conversation

@ZhijieWang
Copy link

I added a button to allow download the current paragraph data as csv

Copy link
Contributor

Choose a reason for hiding this comment

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

This part was actually from a new commit, and should not be removed, my guess is that your branch is not up to date with master.

Copy link
Author

Choose a reason for hiding this comment

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

I will pull from master and update the branch

@corneadoug
Copy link
Contributor

Hi,
Thanks for the contribution, I will review your PR.
For PR that modifies Zeppelin UI, it is better to join a screenshot so that people can see the change without building from your code.
On another note, it seems your branch is not up to date with master branch.

@ZhijieWang
Copy link
Author

The change is to the right corner, a cloud download image. I would suggest to update documentations for how to contribute and how to build. It was quite a code diving.

And thank you for your review.

screenshot from 2015-06-01 21 48 58

@corneadoug
Copy link
Contributor

We are currently in the process of refactoring with the PR#56
So it should get better afterwards :)
Let me play with it a bit, we got a similar PR before, but I would rather use a non-library approach if possible.

@corneadoug
Copy link
Contributor

Here is my test so far:

Test

  • Works for table data to CSV
  • Kinda works for non-csv type of data, but the file is still .csv and while my test didn't make the stack/text separated like a CSV, it could easily happen.
  • There is tooltip on the button

Cross-browser

  • Opera - Launch the download but no default name and format for the file
  • Chrome - works and download as data.csv
  • Firefox - Not working
  • Safari - Open the file in the Browser instead of download
  • IE 10 - Well somehow I can't make Zeppelin run on it anymore :p

Cross-browser for this kind of download is really a pain in the ass, that's why we tried using jquery-datatables and tabletools before.

Discussion

  • Position of the button? (I actually like it there)
  • Should the download data works only for table type of data?
  • Should the CSV be quoted? Should we use a library for the transformation?

@ZhijieWang
Copy link
Author

@corneadoug Thank you for your review.
Test

Works for table data to CSV
Kinda works for non-csv type of data, but the file is still .csv and while my test didn't make the stack/text separated like a CSV, it could easily happen.
There is tooltip on the button

I implemented intended for csv download. I believe if the user want to download any plain text output, they can simple copy and paste.

Cross-browser
I think we should establish a baseline -- allow user to be able to download via right click "save as" or directly launch a download prompt. With some many browsers out there, it is hard to make every feature work. Zeppelin project should have "recommended" or "supported browsers". And unfortunately I don't have IE anymore, so I can't test for that.

Position of the button? (I actually like it there)
Feel free to move it around, i set it to show up when paragraph status == "FINISHED"
Should the download data works only for table type of data?
I implemented intended for csv download. I believe if the user want to download any plain text output, they can simple copy and paste. Maybe when data is not in table format, the button should download the msg value and let end user to do the transformation.
Should the CSV be quoted? Should we use a library for the transformation?
I think we should, I only had numeric value at the time of implementation. If the values are string with white space, it could cause problem.

@qinzl1
Copy link

qinzl1 commented Aug 23, 2016

only 1000 rows download, if i can config more rows for daowload

@ZhijieWang
Copy link
Author

@qinzl1 , I don't think the limitation of how many rows to download is specified by my download button. The download button only converts existing JSON response retrieved from Zeppelin server and converted in to CSV format with UI button. You should look at which specific interpreter you are using. Most example set the max row to display for dataframe or result for 1000.
For example, file

[
  {
    "group": "python",
    "name": "python",
    "className": "org.apache.zeppelin.python.PythonInterpreter",
    "properties": {
      "zeppelin.python": {
        "envName": null,
        "propertyName": "zeppelin.python",
        "defaultValue": "python",
        "description": "Python directory. It is set to python by default.(assume python is in your $PATH)"
      },
      "zeppelin.python.maxResult": {
        "envName": null,
        "propertyName": "zeppelin.python.maxResult",
        "defaultValue": "1000",
        "description": "Max number of dataframe rows to display."
      }
    }
  },
  {
    "group": "python",
    "name": "sql",
    "className": "org.apache.zeppelin.python.PythonInterpreterPandasSql",
    "properties": { }
  }
]```

However, be aware of how many rows you are requesting. The more rows you request, the more memory it uses to build the JSON response and the longer it takes for the browser to do the file format conversion. 

I am not entirely sure how optimized is the JSON parsing and encoding library used by Zeppelin or what library it is. Based on what I have seen, it seems the default library and direct toString() conversion.

egorklimov pushed a commit to Tinkoff/zeppelin that referenced this pull request Sep 18, 2019
…nner_update.fix_copy to V_1.0.0

* commit 'e224b9093eaef130b1c0c6d151b54e50a301f50e':
  [ZP-415] fix note copy
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.

3 participants