[Zeppelin-1001] Take care of comma/tab escape in csv/tsv download by meenakshisekar · Pull Request #1445 · apache/zeppelin

meenakshisekar · 2016-09-21T07:22:17Z

What is this PR for?

When the data is downloaded as CSV/TSV, the comma/tab in the actual data has to be handled so that they come exactly as the same data when downloaded.

What type of PR is it?

[Bug Fix]

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1001

How should this be tested?

Modify the data to be loaded to have a comma.
Create a paragraph to pull up that data and display.
Now click on the download as CSV/TSV button in the tool bar.
Once the data is downloaded verify whether the original data is unaltered and the comma is escaped.

Screenshots (if appropriate)

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

…ble quotes so that they are escaped.

felixcheung · 2016-09-23T03:24:22Z

 var dsvRow = '';
 for (var index in row) {
- dsvRow += row[index].value + delimiter;
+ var stringValue = (row[index].value).toString();


it would need to handle other cases too. to quote here
"CSV implementations permit them, often by requiring " (double quote) characters around values that contain reserved characters (such as commas, double quotes, or less commonly, newlines). Embedded double quote characters may then be represented by a pair of consecutive double quotes,[10] or by prefixing an escape character such as a backslash (for example in Sybase Central)."
For instance, when the stringValue has " or \n

corneadoug · 2016-09-23T07:06:48Z

@felixcheung So you want him to escape the " and \ from the string also?
Otherwise, I tested it, LGTM
CI is failing, but I think that if @meenakshisekar rebase his branch it might be green

felixcheung · 2016-09-23T07:08:17Z

I believe so.

corneadoug · 2016-09-26T06:43:10Z

@meenakshisekar Could you take care of that?

meenakshisekar · 2016-09-26T06:49:58Z

@corneadoug

I'm working towards that. Shall let you know once it is done.
Adding on further @corneadoug
I understand from the above conversation, that when the input to CSV contains double quote or \ it should be retained in the CSV file downloaded.

Confirm this understanding.
If the above is right, i see that the string and \ and " are already escaped when downloaded. I have attached a sample for your reference. Please let me know if you are expecting something different.

]

meenakshisekar · 2016-09-28T05:28:59Z

@corneadoug

Let me know in case of any issues or queries.

Thanks,
Meenakshi

meenakshisekar · 2016-09-28T05:36:01Z

Re opening the pull request to run CI build.

meenakshisekar · 2016-09-28T06:55:48Z

Re opening the pull request to run CI build.

meenakshisekar · 2016-09-28T07:43:06Z

Re opening the pull request to run CI build.

meenakshisekar · 2016-09-28T09:02:13Z

CI build passed!

corneadoug · 2016-09-29T01:40:59Z

Tested using

%sh echo -e "%table name\tval\nhello\thello\you\"good\nhello2\theelow,world"

LGTM

felixcheung · 2016-09-29T06:19:20Z

- dsvRow += row[index].value + delimiter;
+ var stringValue = (row[index].value).toString();
+ if (stringValue.contains(delimiter)) {
+ dsvRow += stringValue.replace(stringValue, '"' + stringValue + '"') + delimiter;


this is a bit odd? why don't we
dsvRow += '"' + stringValue + '"' + delimiter;

@meenakshisekar actually agree with felix, same result, so better use the easy one

@corneadoug
Have done the changes and committed. Please go ahead with merge.

corneadoug · 2016-09-29T06:20:45Z

Merging if there is no more discussions

corneadoug · 2016-09-30T02:31:47Z

+ var stringValue = (row[index].value).toString();
+ if (stringValue.contains(delimiter)) {
+ dsvRow += '"' + stringValue + '"' + delimiter;
+ } else {


Build is failing on indentation error here

corneadoug · 2016-09-30T02:32:18Z

@meenakshisekar There is an indent error breaking the build, can you fix it?
Then, merging this one

corneadoug · 2016-10-02T02:13:21Z

@meenakshisekar Could you fix the indent issue?
I want to merge #1469 but since its refactoring job, it would need to be rebased after yours

meenakshisekar · 2016-10-03T02:13:28Z

@corneadoug
working on this

corneadoug · 2016-10-03T04:55:24Z

Ci failure is irrelevant, Merging

corneadoug · 2016-10-03T05:38:52Z

@meenakshisekar Can you tell me your JIRA username?

meenakshisekar · 2016-10-03T06:12:30Z

@corneadoug
My jira user name is meenakshisekar

corneadoug · 2016-10-03T06:20:35Z

@meenakshisekar I can't find you with this username

meenakshisekar · 2016-10-03T06:27:06Z

OOPS !
It is
meenakshi.chandrasekar@imaginea.com

### What is this PR for? This PR backports #1445 to branch-0.6 since using `./dev/test_zeppelin_pr.py` throws alert for backporting it. ### What type of PR is it? Bug Fix Author: meenakshisekar <meenakshi.chandrasekar@imaginea.com> Closes#1484 from minahlee/branch-0.6_ZEPPELIN-1001 and squashes the following commits: 58fda8e [meenakshisekar] Formatting errors corrected for build f5c7f61 [meenakshisekar] Changes committed as per review comments in PR e404075 [meenakshisekar] Altered the code as per PR suggestion 1465 574803c [meenakshisekar] Zeppelin-1001 Modified the data with comma/tab to be surronded by double quotes so that they are escaped.

### What is this PR for? When the data is downloaded as CSV/TSV, the comma/tab in the actual data has to be handled so that they come exactly as the same data when downloaded. ### What type of PR is it? [Bug Fix] ### Todos * [ ] - Task ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1001 ### How should this be tested? Modify the data to be loaded to have a comma. Create a paragraph to pull up that data and display. Now click on the download as CSV/TSV button in the tool bar. Once the data is downloaded verify whether the original data is unaltered and the comma is escaped. ### 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: meenakshisekar <meenakshi.chandrasekar@imaginea.com> Closesapache#1445 from meenakshisekar/ZEPPELIN-1001 and squashes the following commits: a4a2009 [meenakshisekar] Formatting errors corrected for build 5e14da8 [meenakshisekar] review comments fixed ce27b82 [meenakshisekar] Changes committed as per review comments in PR 2f7d78a [meenakshisekar] Altered the code as per PR suggestion 1465 657dcbe [meenakshisekar] Zeppelin-1001 Modified the data with comma/tab to be surronded by double quotes so that they are escaped. 73b1868 [meenakshisekar] Zeppelin-1001 Modified the data with comma/tab to be surronded by double quotes so that they are escaped.

meenakshisekar added 2 commits September 19, 2016 16:46

Zeppelin-1001 Modified the data with comma/tab to be surronded by dou…
73b1868
…ble quotes so that they are escaped.

Zeppelin-1001 Modified the data with comma/tab to be surronded by dou…
657dcbe
…ble quotes so that they are escaped.

felixcheung reviewed Sep 23, 2016
View reviewed changes

meenakshisekar closed this Sep 28, 2016

meenakshisekar reopened this Sep 28, 2016

meenakshisekar closed this Sep 28, 2016

meenakshisekar reopened this Sep 28, 2016

meenakshisekar closed this Sep 28, 2016

meenakshisekar reopened this Sep 28, 2016

felixcheung reviewed Sep 29, 2016
View reviewed changes

meenakshisekar added 3 commits September 29, 2016 12:29

Altered the code as per PR suggestion 1465
2f7d78a

Changes committed as per review comments in PR
ce27b82

review comments fixed
5e14da8

meenakshisekar closed this Sep 29, 2016

meenakshisekar reopened this Sep 29, 2016

corneadoug reviewed Sep 30, 2016
View reviewed changes

Formatting errors corrected for build
a4a2009

asfgit closed this in 3017730Oct 3, 2016

minahlee mentioned this pull request Oct 5, 2016
[branch-0.6] Take care of comma/tab escape in csv/tsv download #1484
Closed

Uh oh!

Conversation

meenakshisekar commented Sep 21, 2016

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

felixcheungSep 23, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corneadoug commented Sep 23, 2016

Uh oh!

felixcheung commented Sep 23, 2016 via email

Uh oh!

corneadoug commented Sep 26, 2016

Uh oh!

meenakshisekar commented Sep 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meenakshisekar commented Sep 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meenakshisekar commented Sep 28, 2016

Uh oh!

meenakshisekar commented Sep 28, 2016

Uh oh!

meenakshisekar commented Sep 28, 2016

Uh oh!

meenakshisekar commented Sep 28, 2016

Uh oh!

corneadoug commented Sep 29, 2016

Uh oh!

felixcheungSep 29, 2016

Choose a reason for hiding this comment

Uh oh!

corneadougSep 29, 2016

Choose a reason for hiding this comment

Uh oh!

meenakshisekarSep 29, 2016

Choose a reason for hiding this comment

Uh oh!

corneadoug commented Sep 29, 2016

Uh oh!

corneadougSep 30, 2016

Choose a reason for hiding this comment

Uh oh!

corneadoug commented Sep 30, 2016

Uh oh!

corneadoug commented Oct 2, 2016

Uh oh!

meenakshisekar commented Oct 3, 2016

Uh oh!

corneadoug commented Oct 3, 2016

Uh oh!

corneadoug commented Oct 3, 2016

Uh oh!

meenakshisekar commented Oct 3, 2016

Uh oh!

corneadoug commented Oct 3, 2016

Uh oh!

meenakshisekar commented Oct 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felixcheungSep 23, 2016•
edited
Loading

meenakshisekar commented Sep 26, 2016•
edited
Loading

meenakshisekar commented Sep 28, 2016•
edited
Loading