You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by jkirsch <gi...@git.apache.org> on 2014/11/06 21:53:38 UTC
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
GitHub user jkirsch opened a pull request:
https://github.com/apache/incubator-flink/pull/187
[FLINK-1223] Allow value escaping in CSV files.
Strings can now contain " quoted characters
Skip trailing whitespace after quote
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jkirsch/incubator-flink csv
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-flink/pull/187.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #187
----
commit 1c03d901b47c8a7147fc5ed17ca36f5b1791af99
Author: Johannes <jk...@gmail.com>
Date: 2014-11-06T19:19:56Z
[FLINK-1223] Allow value escaping in CSV files.
Strings can now contain " quoted characters
Skip trailing whitespace after quote
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/incubator-flink/pull/187#issuecomment-62530397
OK. I'm merging this in the current batch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/187#discussion_r20003612
--- Diff: flink-core/src/main/java/org/apache/flink/types/parser/StringParser.java ---
@@ -30,7 +34,14 @@
private static final byte WHITESPACE_TAB = (byte) '\t';
private static final byte QUOTE_DOUBLE = (byte) '"';
-
+
+ private static final Set<Byte> trailingCheckSet = Sets.newHashSet(
--- End diff --
The parsers are often stressed badly when reading large CSV files, so we think a lot about performance here.
I am wondering if a hash set is the best choice to check for these three elements. Computing hash, table lookup, entry lookup, comparison, ...
Might be cheaper to just hardwire the check for those types...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-flink/pull/187
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by jkirsch <gi...@git.apache.org>.
Github user jkirsch commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/187#discussion_r20007010
--- Diff: flink-core/src/main/java/org/apache/flink/types/parser/StringParser.java ---
@@ -30,7 +34,14 @@
private static final byte WHITESPACE_TAB = (byte) '\t';
private static final byte QUOTE_DOUBLE = (byte) '"';
-
+
+ private static final Set<Byte> trailingCheckSet = Sets.newHashSet(
--- End diff --
You are right
https://microbenchmarks.appspot.com/runs/b3ab8918-7226-4527-b019-c622a728d144
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/incubator-flink/pull/187#issuecomment-62361647
Looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by jkirsch <gi...@git.apache.org>.
Github user jkirsch commented on the pull request:
https://github.com/apache/incubator-flink/pull/187#issuecomment-62133790
Thanks for having a look at the code
Indeed the Set approach is nicer to read but there is a performance penalty incurred
I just ran a simple caliper benchmark and on my machine the set approach is about 10x slower, so I fixed that.
https://microbenchmarks.appspot.com/runs/b3ab8918-7226-4527-b019-c622a728d144
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: [FLINK-1223] Allow value escaping in...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/incubator-flink/pull/187#issuecomment-62124858
Looks good. I have one inline comment/question, though
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---