Skip to content

Feature/extensible parsers#1

Merged
blrnw3 merged 2 commits into
quartethealth:masterfrom
blrnw3:feature/extensible_parsers
Feb 12, 2016
Merged

Feature/extensible parsers#1
blrnw3 merged 2 commits into
quartethealth:masterfrom
blrnw3:feature/extensible_parsers

Conversation

@blrnw3

@blrnw3 blrnw3 commented Feb 11, 2016

Copy link
Copy Markdown

blrnw3 and others added 2 commits February 10, 2016 17:59
Currently a lot of functionality is hidden behind private access modifiers making it difficult to extend the functionality of the parsers without copying large chunks of code. These small changes make re-use much easier.
Was able to reinstate some of the access modifiers and instead extracted out extensible code from those methods into new overridable methods.
Added some hierarchy to the parsers/readers so they can be switched out.
@mustafashabib

Copy link
Copy Markdown

@thyming @pheuter Hopefully you connected with @blrnw3 about this because I think I utterly confused myself - but I think if this is merged then you can import it into your projects and all is right in the world.

@blrnw3

blrnw3 commented Feb 12, 2016

Copy link
Copy Markdown
Author

Sorry, that was my fault for not explaining it well at all. I'm going to merge this in now since it is the same PR as the one for databricks-spark-csv: databricks#259

So if you guys have any comments they'd be better off going on that PR.

@blrnw3

blrnw3 commented Feb 12, 2016

Copy link
Copy Markdown
Author

@mustafashabib Please can you give me write access to this repo? Thanks

`remote: Permission to quartethealth/spark-csv.git denied to blrnw3.
fatal: unable to access 'https://github.com/quartethealth/spark-csv.git/': The requested URL returned error: 403
'

@mustafashabib

Copy link
Copy Markdown

Sorry about that Ben - can you try again?

On Feb 12, 2016, at 3:42 PM, Ben LR <notifications@github.com mailto:notifications@github.com> wrote:

@mustafashabib https://github.com/mustafashabib Please can you give me write access to this repo? Thanks

`remote: Permission to quartethealth/spark-csv.git denied to blrnw3.
fatal: unable to access 'https://github.com/quartethealth/spark-csv.git/ https://github.com/quartethealth/spark-csv.git/': The requested URL returned error: 403
'


Reply to this email directly or view it on GitHub #1 (comment).

blrnw3 pushed a commit that referenced this pull request Feb 12, 2016
@blrnw3 blrnw3 merged commit 6a9e50c into quartethealth:master Feb 12, 2016
@blrnw3

blrnw3 commented Feb 12, 2016

Copy link
Copy Markdown
Author

Works now. thanks!

}

/**
* Allows for greater extensibility

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment and the one above don't really add anything IMO

@thyming

thyming commented Feb 17, 2016

Copy link
Copy Markdown

Can you create tests for this?

import org.apache.spark.rdd.RDD

private[csv] object TextFile {
object TextFile {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why is this access modifier changed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@thyming

thyming commented Feb 17, 2016

Copy link
Copy Markdown

is there a different PR that you're preparing for dealing with fixed width data?

@blrnw3

blrnw3 commented Feb 17, 2016

Copy link
Copy Markdown
Author

Thanks for the comments. I'll work on those changes now.
I'm not sure there's any value in extra tests for this, as the changes are pretty low-level / implementation-oriented. I've verified that the existing tests run ok.

The fixed-width parser PR: quartethealth/spark-fixedwidth#1

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