You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Sean Hsuan-Yi Chu <hs...@usc.edu> on 2015/08/28 18:32:27 UTC
Review Request 37893: DRILL-3718: TSV reader fails when "" appears
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
Review request for drill, Aman Sinha, Jinfeng Ni, and Mehant Baid.
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV file, if the TextReader reads the a quote, it would keep scanning until it sees the second quote.
And stop at the second quote to construct a field covering the text within the two quotes
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Aug. 28, 2015, 8:54 p.m., Jacques Nadeau wrote:
> > -1 This code is tab specific. This should be corrected to be generic.
Not sure what you meant by "generic"
Did you mean the tab can be replaced with other delimiters such as '|', ',' etc?
If that speculation is correct, please see the comment above that if-statement (where I added additional constraint)
The logic surrounded in the if-statement is used to ignore whitespaces
And the second condition (i.e., ch <= ' ') would force ignoring '\t'
However, it is not correct because if your delimiter is '\t' (tsv files), '\t' should not be ignored.
(I found some insufficiency in the code so please read the new patch)
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96929
-----------------------------------------------------------
On Aug. 28, 2015, 8:47 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 8:47 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96929
-----------------------------------------------------------
-1 This code is tab specific. This should be corrected to be generic.
- Jacques Nadeau
On Aug. 28, 2015, 8:47 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 8:47 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Khurram Faraaz <kf...@maprtech.com>.
Hsuan,
A test with 6 rows and 60 fields per row is not the best test (at least to
evaluate the performance).
Please note a previous comment in this email chain mentions the use of a
2gb CSV file for performance testing purposes previously.
I would suggest that you should run your patch of a larger dataset and
then evaluate performance regressions if any.
On Fri, Aug 28, 2015 at 7:32 PM, Sean Hsuan-Yi Chu <hs...@usc.edu> wrote:
>
>
> > On Aug. 28, 2015, 9:42 p.m., Jacques Nadeau wrote:
> > > Can you please start out by discussing an approach. I need to look at
> this in more detail but I think you're trying to correct a symptom rather
> than the root problem. This is generic code (sometimes used for tab
> delimited, sometimes for comma, somtimes for space or pipe). As such, I
> don't expect to see a tab character in the common code. If we need to
> change conditions for this situation, we need to figure out the right way.
> For example, what if someone uses a space delimiter for fields? It seems
> like we're going to hit a similar problem.
> > >
> > > Additionally, this is extremely performance sensitive code. Before
> doing submitting any change on this code, you need to do performance
> testing. I used a 2gb CSV file for performance testing purposes previously.
> >
> > Sean Hsuan-Yi Chu wrote:
> > Let me first explain the symptom.
> >
> > The problem happens when we have double-quoted fields in the file.
> >
> > If there are whitespaces after the second quote (e.g., “current
> field” ,next field),
> >
> > TextReader will try to ignore them (the if-statement is doing this
> task).
> >
> > However, it is incorrect because those whitespaces could be used as
> delimiter.
> >
> >
> >
> >
> >
> > My solution:
> >
> > Thus, my solution is to add one more condition as follows:
> >
> > “if character is whitespace and it is not used as delimiter”
> >
> > Besides, I found a new writing which would be more generic and not
> need to have ‘\t’ in the code:
> >
> > if (ch != newLine && ch <= ' ' && ch != delimiter)
> >
> > ——
> >
> > Now I am doing some performance testing. Once it is done and makes
> sense, I will give the result and upload the latest patch.
>
> I did an experiment and collected statistics here.
>
> 1. Data: a csv file with 6 rows and 60 fields per row.
> 2. Let the two versions of TextReader read the same csv file three times:
> On average, the running times are 394.68 (the one on master) versus 413.11
> (this patch). It is 4% slower. It seems ok?
>
>
> - Sean Hsuan-Yi
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/#review96936
> -----------------------------------------------------------
>
>
> On Aug. 28, 2015, 9:35 p.m., Sean Hsuan-Yi Chu wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/37893/
> > -----------------------------------------------------------
> >
> > (Updated Aug. 28, 2015, 9:35 p.m.)
> >
> >
> > Review request for drill, Jacques Nadeau and Mehant Baid.
> >
> >
> > Bugs: DRILL-3718
> > https://issues.apache.org/jira/browse/DRILL-3718
> >
> >
> > Repository: drill-git
> >
> >
> > Description
> > -------
> >
> > For TSV files, if the TextReader reads a double quote, it would keep
> scanning until it gets the second double quote.
> >
> > However, even getting the second double quote, the current reader will
> keep going in order to trim the space (i.e., ' ').
> >
> > In tsv, there is no need to trim '\t' (tab), which is used to separate
> fields.
> >
> >
> > Diffs
> > -----
> >
> >
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java
> 3899509
> > exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java
> 6b74ecf
> > exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/37893/diff/
> >
> >
> > Testing
> > -------
> >
> > All
> >
> >
> > Thanks,
> >
> > Sean Hsuan-Yi Chu
> >
> >
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Aug. 28, 2015, 9:42 p.m., Jacques Nadeau wrote:
> > Can you please start out by discussing an approach. I need to look at this in more detail but I think you're trying to correct a symptom rather than the root problem. This is generic code (sometimes used for tab delimited, sometimes for comma, somtimes for space or pipe). As such, I don't expect to see a tab character in the common code. If we need to change conditions for this situation, we need to figure out the right way. For example, what if someone uses a space delimiter for fields? It seems like we're going to hit a similar problem.
> >
> > Additionally, this is extremely performance sensitive code. Before doing submitting any change on this code, you need to do performance testing. I used a 2gb CSV file for performance testing purposes previously.
>
> Sean Hsuan-Yi Chu wrote:
> Let me first explain the symptom.
>
> The problem happens when we have double-quoted fields in the file.
>
> If there are whitespaces after the second quote (e.g., “current field” ,next field),
>
> TextReader will try to ignore them (the if-statement is doing this task).
>
> However, it is incorrect because those whitespaces could be used as delimiter.
>
>
>
>
>
> My solution:
>
> Thus, my solution is to add one more condition as follows:
>
> “if character is whitespace and it is not used as delimiter”
>
> Besides, I found a new writing which would be more generic and not need to have ‘\t’ in the code:
>
> if (ch != newLine && ch <= ' ' && ch != delimiter)
>
> ——
>
> Now I am doing some performance testing. Once it is done and makes sense, I will give the result and upload the latest patch.
I did an experiment and collected statistics here.
1. Data: a csv file with 6 rows and 60 fields per row.
2. Let the two versions of TextReader read the same csv file three times: On average, the running times are 394.68 (the one on master) versus 413.11 (this patch). It is 4% slower. It seems ok?
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96936
-----------------------------------------------------------
On Aug. 28, 2015, 9:35 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 9:35 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Aug. 28, 2015, 9:42 p.m., Jacques Nadeau wrote:
> > Can you please start out by discussing an approach. I need to look at this in more detail but I think you're trying to correct a symptom rather than the root problem. This is generic code (sometimes used for tab delimited, sometimes for comma, somtimes for space or pipe). As such, I don't expect to see a tab character in the common code. If we need to change conditions for this situation, we need to figure out the right way. For example, what if someone uses a space delimiter for fields? It seems like we're going to hit a similar problem.
> >
> > Additionally, this is extremely performance sensitive code. Before doing submitting any change on this code, you need to do performance testing. I used a 2gb CSV file for performance testing purposes previously.
Let me first explain the symptom.
The problem happens when we have double-quoted fields in the file.
If there are whitespaces after the second quote (e.g., “current field” ,next field),
TextReader will try to ignore them (the if-statement is doing this task).
However, it is incorrect because those whitespaces could be used as delimiter.
My solution:
Thus, my solution is to add one more condition as follows:
“if character is whitespace and it is not used as delimiter”
Besides, I found a new writing which would be more generic and not need to have ‘\t’ in the code:
if (ch != newLine && ch <= ' ' && ch != delimiter)
——
Now I am doing some performance testing. Once it is done and makes sense, I will give the result and upload the latest patch.
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96936
-----------------------------------------------------------
On Aug. 28, 2015, 9:35 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 9:35 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Aug. 28, 2015, 9:42 p.m., Jacques Nadeau wrote:
> > Can you please start out by discussing an approach. I need to look at this in more detail but I think you're trying to correct a symptom rather than the root problem. This is generic code (sometimes used for tab delimited, sometimes for comma, somtimes for space or pipe). As such, I don't expect to see a tab character in the common code. If we need to change conditions for this situation, we need to figure out the right way. For example, what if someone uses a space delimiter for fields? It seems like we're going to hit a similar problem.
> >
> > Additionally, this is extremely performance sensitive code. Before doing submitting any change on this code, you need to do performance testing. I used a 2gb CSV file for performance testing purposes previously.
>
> Sean Hsuan-Yi Chu wrote:
> Let me first explain the symptom.
>
> The problem happens when we have double-quoted fields in the file.
>
> If there are whitespaces after the second quote (e.g., “current field” ,next field),
>
> TextReader will try to ignore them (the if-statement is doing this task).
>
> However, it is incorrect because those whitespaces could be used as delimiter.
>
>
>
>
>
> My solution:
>
> Thus, my solution is to add one more condition as follows:
>
> “if character is whitespace and it is not used as delimiter”
>
> Besides, I found a new writing which would be more generic and not need to have ‘\t’ in the code:
>
> if (ch != newLine && ch <= ' ' && ch != delimiter)
>
> ——
>
> Now I am doing some performance testing. Once it is done and makes sense, I will give the result and upload the latest patch.
>
> Sean Hsuan-Yi Chu wrote:
> I did an experiment and collected statistics here.
>
> 1. Data: a csv file with 6 rows and 60 fields per row.
> 2. Let the two versions of TextReader read the same csv file three times: On average, the running times are 394.68 (the one on master) versus 413.11 (this patch). It is 4% slower. It seems ok?
My bad. It is 6 million rows. The size of the file is 3 gb.
(I do apologize. Drill should never have spent 400 secs to scan just 6 rows...)
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96936
-----------------------------------------------------------
On Aug. 29, 2015, 3:10 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 3:10 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review96936
-----------------------------------------------------------
Can you please start out by discussing an approach. I need to look at this in more detail but I think you're trying to correct a symptom rather than the root problem. This is generic code (sometimes used for tab delimited, sometimes for comma, somtimes for space or pipe). As such, I don't expect to see a tab character in the common code. If we need to change conditions for this situation, we need to figure out the right way. For example, what if someone uses a space delimiter for fields? It seems like we're going to hit a similar problem.
Additionally, this is extremely performance sensitive code. Before doing submitting any change on this code, you need to do performance testing. I used a 2gb CSV file for performance testing purposes previously.
- Jacques Nadeau
On Aug. 28, 2015, 9:35 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 9:35 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review98976
-----------------------------------------------------------
Ship it!
Ship It!
- Jacques Nadeau
On Sept. 14, 2015, 10:52 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 10:52 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java e88e2b3
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
> exec/java-exec/src/test/resources/bootstrap-storage-plugins.json 4a7a53f
> exec/java-exec/src/test/resources/store/text/WithQuote.ssv PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuote.tbl PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> pom.xml c17e612
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Sept. 14, 2015, 10:52 p.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
Rebased again
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java e88e2b3
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
exec/java-exec/src/test/resources/bootstrap-storage-plugins.json 4a7a53f
exec/java-exec/src/test/resources/store/text/WithQuote.ssv PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuote.tbl PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
pom.xml c17e612
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 4:33 p.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
Uploaded the latest patch
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
exec/java-exec/src/test/resources/bootstrap-storage-plugins.json 4a7a53f
exec/java-exec/src/test/resources/store/text/WithQuote.ssv PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuote.tbl PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
pom.xml 8d4b318
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Sept. 9, 2015, 3:11 p.m., Jacques Nadeau wrote:
> > Most of this looks good. Let me restate my additional request as I think I'm being unclear.
> >
> > You are currently testing tsv files (tab-delimited files). I want to you test the same type of conditions with another type of delimiter. For example, space delimited files (e.g. .ssv). This will verify that your solution works for multiple delimiter types.
Oh... I uploaded the previous version.
Please check the latest one. (I also updated the pom file and boot-strap to make unit test recognize .ssv)
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review98199
-----------------------------------------------------------
On Sept. 9, 2015, 3:06 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 3:06 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review98199
-----------------------------------------------------------
Most of this looks good. Let me restate my additional request as I think I'm being unclear.
You are currently testing tsv files (tab-delimited files). I want to you test the same type of conditions with another type of delimiter. For example, space delimited files (e.g. .ssv). This will verify that your solution works for multiple delimiter types.
- Jacques Nadeau
On Sept. 9, 2015, 3:06 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 3:06 p.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 3:06 p.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
Addressed comments
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 12:09 a.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
Addressed reviewer's comment
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Sept. 8, 2015, 3:47 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java, line 1105
> > <https://reviews.apache.org/r/37893/diff/3/?file=1059076#file1059076line1105>
> >
> > Please move text tests to their own file. I believe we already have a TestTextReader.
> >
> > Can you also please add another file with a different format to prove that this fix works for multiple delimiters?
> >
> > Lastly, you should a small comment to the condition where you've added it.
>
> Sean Hsuan-Yi Chu wrote:
> 1. Done
> 2. By "multiple", I am assuming you meant like: "a"\t\t"a"? (Please see the diff)
> 3. Done
>
> Jacques Nadeau wrote:
> I mean try with something other than a tab as the delimiter. For example, try it with a space delimiter.
Done
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review97962
-----------------------------------------------------------
On Sept. 9, 2015, 12:09 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 12:09 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
> On Sept. 8, 2015, 3:47 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java, line 1105
> > <https://reviews.apache.org/r/37893/diff/3/?file=1059076#file1059076line1105>
> >
> > Please move text tests to their own file. I believe we already have a TestTextReader.
> >
> > Can you also please add another file with a different format to prove that this fix works for multiple delimiters?
> >
> > Lastly, you should a small comment to the condition where you've added it.
>
> Sean Hsuan-Yi Chu wrote:
> 1. Done
> 2. By "multiple", I am assuming you meant like: "a"\t\t"a"? (Please see the diff)
> 3. Done
I mean try with something other than a tab as the delimiter. For example, try it with a space delimiter.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review97962
-----------------------------------------------------------
On Sept. 9, 2015, 12:09 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 12:09 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java d15cff2
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestNewTextReader.java e63e528
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
> exec/java-exec/src/test/resources/store/text/WithQuoteMultiDelimiters.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On Sept. 8, 2015, 3:47 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java, line 1105
> > <https://reviews.apache.org/r/37893/diff/3/?file=1059076#file1059076line1105>
> >
> > Please move text tests to their own file. I believe we already have a TestTextReader.
> >
> > Can you also please add another file with a different format to prove that this fix works for multiple delimiters?
> >
> > Lastly, you should a small comment to the condition where you've added it.
1. Done
2. By "multiple", I am assuming you meant like: "a"\t\t"a"? (Please see the diff)
3. Done
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review97962
-----------------------------------------------------------
On Aug. 29, 2015, 3:10 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 3:10 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/#review97962
-----------------------------------------------------------
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java (line 1105)
<https://reviews.apache.org/r/37893/#comment154091>
Please move text tests to their own file. I believe we already have a TestTextReader.
Can you also please add another file with a different format to prove that this fix works for multiple delimiters?
Lastly, you should a small comment to the condition where you've added it.
- Jacques Nadeau
On Aug. 29, 2015, 3:10 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37893/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 3:10 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Mehant Baid.
>
>
> Bugs: DRILL-3718
> https://issues.apache.org/jira/browse/DRILL-3718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
>
> However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
>
> In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
> exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37893/diff/
>
>
> Testing
> -------
>
> All
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Aug. 29, 2015, 3:10 a.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
A new patch without using '\t'; This new version passed all the test
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 9:35 p.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Changes
-------
new patch
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 37893: DRILL-3718: TSV reader fails when "" appears
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37893/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 8:47 p.m.)
Review request for drill, Jacques Nadeau and Mehant Baid.
Bugs: DRILL-3718
https://issues.apache.org/jira/browse/DRILL-3718
Repository: drill-git
Description (updated)
-------
For TSV files, if the TextReader reads a double quote, it would keep scanning until it gets the second double quote.
However, even getting the second double quote, the current reader will keep going in order to trim the space (i.e., ' ').
In tsv, there is no need to trim '\t' (tab), which is used to separate fields.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/TextReader.java 3899509
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 6b74ecf
exec/java-exec/src/test/resources/store/text/WithQuote.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/37893/diff/
Testing
-------
All
Thanks,
Sean Hsuan-Yi Chu