You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@crunch.apache.org by Josh Wills <jw...@cloudera.com> on 2012/11/21 03:31:28 UTC
Review Request: Latest take on CRUNCH-97, text parsing lib for Crunch
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8151/
-----------------------------------------------------------
Review request for crunch.
Description
-------
Latest and greatest rev of the extraction library for text parsing. I ended up refactoring the approach so that we could support nested parsing (e.g., using different Scanner instances for different parts of a line) and collections of items on a single line.
This addresses bug CRUNCH-97.
https://issues.apache.org/jira/browse/CRUNCH-97
Diffs
-----
crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/8151/diff/
Testing
-------
Unit tests so far, still gathering feedback on the approach.
Thanks,
Josh Wills
Re: Review Request: Latest take on CRUNCH-97, text parsing lib for Crunch
Posted by Josh Wills <jo...@gmail.com>.
Hey Matthias,
Your use case makes a lot of sense to me. I'm still recovering from a
business trip last week, but I'll take a look at the latest patch tomorrow
morning.
Josh
On Fri, Nov 30, 2012 at 12:23 PM, Matthias Friedrich <ma...@mafr.de> wrote:
> Hi Josh,
>
> sorry for taking so long. I've checked the code and I'm confident that
> it works as intended. There are a few things about handling and
> reporting missing data that I'd like to look into, but what we have
> now is great stuff already.
>
> Based on my own use cases I'd like to discuss a slightly different
> solution. I'm not sure if my requirements are esoteric, but perhaps
> that's what others need, too.
>
> The data I work with usually comes in CSV format with many rows and
> several hundreds of attributes. I have a schema for the file so I can
> easily map column names to column numbers. When I process the file I
> am often interested in only a small subset of columns. I would want
> a parsing library to take a row of the file and give me a Tuple with
> only the columns I'm interested in. Since we don't have a NamedTuple
> abstraction, I don't want the Tuple to contain too much data that
> I don't need.
>
> This is possible with the current implementation, but the Scanner
> stuff looks a bit unwieldy when it comes to skipping data. Here's how
> I would like to specify the extraction process:
>
> Parse.parse(data, tokenizer,
> xtuple(xstring(0), xint(7), xboolean(3), xdouble(9)))
>
> The int argument specifies the column number to extract the data from.
> This approach would work best if we just take the input record and
> turn it into a sequence of tokens. We could offer alternative
> strategies for tokenizing, like regex for log parsing (we pull out the
> groups specified in the pattern) or simple splitting at a static
> or regex delimiter. The extractors get the sequence of tokens passed
> in and take whatever they need.
>
> I'm a bit busy right now but I'd help out with some code if you want.
> It would probably take a bit until I can make some time though.
>
> What do you think?
>
> Regards,
> Matthias
>
> On Sunday, 2012-11-25, Josh Wills wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/8151/
> > -----------------------------------------------------------
> >
> > (Updated Nov. 25, 2012, 7:21 p.m.)
> >
> >
> > Review request for crunch.
> >
> >
> > Changes
> > -------
> >
> > Incorporated feedback from Matthias and Gabriel; added a bunch of
> javadoc.
> >
> >
> > Description
> > -------
> >
> > Latest and greatest rev of the extraction library for text parsing. I
> ended up refactoring the approach so that we could support nested parsing
> (e.g., using different Scanner instances for different parts of a line) and
> collections of items on a single line.
> >
> >
> > This addresses bug CRUNCH-97.
> > https://issues.apache.org/jira/browse/CRUNCH-97
> >
> >
> > Diffs (updated)
> > -----
> >
> > crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
> > crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
> >
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java
> PRE-CREATION
> >
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java
> PRE-CREATION
> >
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java
> PRE-CREATION
> >
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java
> PRE-CREATION
> > crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java
> PRE-CREATION
> > crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java
> PRE-CREATION
> > crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java
> PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/8151/diff/
> >
> >
> > Testing
> > -------
> >
> > Unit tests so far, still gathering feedback on the approach.
> >
> >
> > Thanks,
> >
> > Josh Wills
> >
>
Re: Review Request: Latest take on CRUNCH-97, text parsing lib for
Crunch
Posted by Matthias Friedrich <ma...@mafr.de>.
Hi Josh,
sorry for taking so long. I've checked the code and I'm confident that
it works as intended. There are a few things about handling and
reporting missing data that I'd like to look into, but what we have
now is great stuff already.
Based on my own use cases I'd like to discuss a slightly different
solution. I'm not sure if my requirements are esoteric, but perhaps
that's what others need, too.
The data I work with usually comes in CSV format with many rows and
several hundreds of attributes. I have a schema for the file so I can
easily map column names to column numbers. When I process the file I
am often interested in only a small subset of columns. I would want
a parsing library to take a row of the file and give me a Tuple with
only the columns I'm interested in. Since we don't have a NamedTuple
abstraction, I don't want the Tuple to contain too much data that
I don't need.
This is possible with the current implementation, but the Scanner
stuff looks a bit unwieldy when it comes to skipping data. Here's how
I would like to specify the extraction process:
Parse.parse(data, tokenizer,
xtuple(xstring(0), xint(7), xboolean(3), xdouble(9)))
The int argument specifies the column number to extract the data from.
This approach would work best if we just take the input record and
turn it into a sequence of tokens. We could offer alternative
strategies for tokenizing, like regex for log parsing (we pull out the
groups specified in the pattern) or simple splitting at a static
or regex delimiter. The extractors get the sequence of tokens passed
in and take whatever they need.
I'm a bit busy right now but I'd help out with some code if you want.
It would probably take a bit until I can make some time though.
What do you think?
Regards,
Matthias
On Sunday, 2012-11-25, Josh Wills wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8151/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2012, 7:21 p.m.)
>
>
> Review request for crunch.
>
>
> Changes
> -------
>
> Incorporated feedback from Matthias and Gabriel; added a bunch of javadoc.
>
>
> Description
> -------
>
> Latest and greatest rev of the extraction library for text parsing. I ended up refactoring the approach so that we could support nested parsing (e.g., using different Scanner instances for different parts of a line) and collections of items on a single line.
>
>
> This addresses bug CRUNCH-97.
> https://issues.apache.org/jira/browse/CRUNCH-97
>
>
> Diffs (updated)
> -----
>
> crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
> crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
> crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
> crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8151/diff/
>
>
> Testing
> -------
>
> Unit tests so far, still gathering feedback on the approach.
>
>
> Thanks,
>
> Josh Wills
>
Re: Review Request: Latest take on CRUNCH-97, text parsing lib for Crunch
Posted by Josh Wills <jw...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8151/
-----------------------------------------------------------
(Updated Nov. 25, 2012, 7:21 p.m.)
Review request for crunch.
Changes
-------
Incorporated feedback from Matthias and Gabriel; added a bunch of javadoc.
Description
-------
Latest and greatest rev of the extraction library for text parsing. I ended up refactoring the approach so that we could support nested parsing (e.g., using different Scanner instances for different parts of a line) and collections of items on a single line.
This addresses bug CRUNCH-97.
https://issues.apache.org/jira/browse/CRUNCH-97
Diffs (updated)
-----
crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/8151/diff/
Testing
-------
Unit tests so far, still gathering feedback on the approach.
Thanks,
Josh Wills
Re: Review Request: Latest take on CRUNCH-97, text parsing lib for Crunch
Posted by Matthias Friedrich <ma...@mafr.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8151/#review13700
-----------------------------------------------------------
crunch/src/main/java/org/apache/crunch/lib/PTables.java
<https://reviews.apache.org/r/8151/#comment29361>
This method doesn't seem to be used anywhere. Remove?
crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java
<https://reviews.apache.org/r/8151/#comment29362>
Missing javadoc
crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java
<https://reviews.apache.org/r/8151/#comment29363>
This class generates lots of warnings in Eclipse; using Extractor<?> instead of just Extractor will shut Eclipse up.
crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java
<https://reviews.apache.org/r/8151/#comment29364>
Needs javadoc describing the contract. I think GoF suggests doExtract() for template methods.
crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java
<https://reviews.apache.org/r/8151/#comment29365>
I think all inner classes here should be private, like in Aggregators. Otherwise we'll add a lot of new classes to our javadocs. Also, this class needs some more stating-the-obvious javadoc ;-)
crunch/src/main/java/org/apache/crunch/lib/text/Parse.java
<https://reviews.apache.org/r/8151/#comment29366>
Final, with private constructor?
crunch/src/main/java/org/apache/crunch/lib/text/Parse.java
<https://reviews.apache.org/r/8151/#comment29367>
Does this need to be public?
crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java
<https://reviews.apache.org/r/8151/#comment29369>
I think a fluent Builder pattern would be more appropriate than a Factory. This would solve the problem of bulky method names like forDelimiterAnSkip().
crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java
<https://reviews.apache.org/r/8151/#comment29368>
As per classic Code Conventions from Sun, attributes should be at the top of the class.
Pretty cool, looking forward to the next round.
It was too late already when I noticed that you can annotate multiple lines, but otherwise I think I could get used to RB ;-)
- Matthias Friedrich
On Nov. 21, 2012, 2:31 a.m., Josh Wills wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8151/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2012, 2:31 a.m.)
>
>
> Review request for crunch.
>
>
> Description
> -------
>
> Latest and greatest rev of the extraction library for text parsing. I ended up refactoring the approach so that we could support nested parsing (e.g., using different Scanner instances for different parts of a line) and collections of items on a single line.
>
>
> This addresses bug CRUNCH-97.
> https://issues.apache.org/jira/browse/CRUNCH-97
>
>
> Diffs
> -----
>
> crunch/src/main/java/org/apache/crunch/lib/PTables.java e788656
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractCompositeExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/AbstractSimpleExtractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractor.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ExtractorStats.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Extractors.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/Parse.java PRE-CREATION
> crunch/src/main/java/org/apache/crunch/lib/text/ScannerFactory.java PRE-CREATION
> crunch/src/test/java/org/apache/crunch/lib/text/ParseTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8151/diff/
>
>
> Testing
> -------
>
> Unit tests so far, still gathering feedback on the approach.
>
>
> Thanks,
>
> Josh Wills
>
>