Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DailySuffixTypedTsv and HourlySuffixTypedTsv. #873

Merged
merged 4 commits into from
May 16, 2014

Conversation

reconditesea
Copy link
Contributor

#870
Directly use DailySuffixTypedTsv in SourceSpec tests.

@@ -42,6 +42,16 @@ class DailySuffixTsv(prefix: String, fs: Fields = Fields.ALL)(override implicit
override val fields = fs
}

object DailySuffixTypedTsv {
def apply(prefix: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting is a little strange, I feel we usually prefer dropping the result to the next line before the arguments

}

class DailySuffixTypedTsv(prefix: String)(override implicit val dateRange: DateRange)
extends DailySuffixSource(prefix, dateRange) with TypedSeperatedFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

TypedSeparatedFile is not what makes something a TypedTsv. If you have this, what you will have is a DailySuffixSource which has an apply method which will create TypedTsv's. ie

(DailySuffixTypedTsv("prefix))("input) would be a TypedTsv. This is obviously not what you want. Waht you want is the thing that TypedSeparatedFile creates, a TypedDelimited

See: https://github.com/twitter/scalding/blob/klin_suffix_typedtsv/scalding-core/src/main/scala/com/twitter/scalding/TypedDelimited.scala?pr=%2Ftwitter%2Fscalding%2Fpull%2F873#L94

Does this make sense? TypedSeparatedFile is something which the TypedTsv, TypedCsv object etc extend for ease of the creation of various TypedDelimited. TypedDelimited is what makes something a TypedTsv. It's a little confusing I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for the clarification.
So extend TypedDelimited with DailySuffixSource should give me the desired DailySuffixTypedTsv I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually TypedDelimited is a class and extends FixedPathSource, so it won't work with DailySuffixSource. Does it make sense to change TypedDelimited to trait without extending any *PathSource? And create a FixedPathTypedDelimited class to replace the current TypedDelimited class?

@jcoveney
Copy link
Contributor

That sounds pretty reasonable!

@reconditesea
Copy link
Contributor Author

@jcoveney Mind taking another look at this?

jcoveney added a commit that referenced this pull request May 16, 2014
Add DailySuffixTypedTsv and HourlySuffixTypedTsv.
@jcoveney jcoveney merged commit 6c61a6f into develop May 16, 2014
@jcoveney jcoveney deleted the klin_suffix_typedtsv branch May 16, 2014 17:56
@jcoveney
Copy link
Contributor

Thanks @reconditesea !

@reconditesea
Copy link
Contributor Author

Thank you @jcoveney!

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.

None yet

2 participants