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 partitionToMap function #1968

Merged
merged 4 commits into from Jun 14, 2019
Merged

Conversation

freyrsae
Copy link
Contributor

It can be annoying to be forced to work with Integers while doing partitions. This is a helper method that takes care of that annoyance by allowing the user to give a set of possible outcomes and a function into that set instead.

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #1968 into master will decrease coverage by 1.71%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1968      +/-   ##
==========================================
- Coverage   69.33%   67.62%   -1.72%     
==========================================
  Files         214      214              
  Lines        6659     6662       +3     
  Branches      365      460      +95     
==========================================
- Hits         4617     4505     -112     
- Misses       2042     2157     +115
Impacted Files Coverage Δ
...in/scala/com/spotify/scio/values/SCollection.scala 90.41% <100%> (+0.13%) ⬆️
...scala/com/spotify/scio/bigquery/BigQueryUtil.scala 50% <0%> (-50%) ⬇️
...la/com/spotify/scio/bigquery/client/QueryOps.scala 0.8% <0%> (-48.39%) ⬇️
.../spotify/scio/bigquery/BigQueryPartitionUtil.scala 25% <0%> (-32.5%) ⬇️
.../spotify/scio/bigquery/client/BigQueryConfig.scala 41.17% <0%> (-29.42%) ⬇️
...scala/com/spotify/scio/bigquery/client/Cache.scala 62.96% <0%> (-18.52%) ⬇️
...la/com/spotify/scio/bigquery/client/TableOps.scala 1.76% <0%> (-16.82%) ⬇️
...com/spotify/scio/bigquery/types/BigQueryType.scala 57.89% <0%> (-15.79%) ⬇️
...la/com/spotify/scio/bigquery/client/BigQuery.scala 28.94% <0%> (-10.53%) ⬇️
...n/scala/com/spotify/scio/bigquery/BigQueryIO.scala 24.66% <0%> (-2.67%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 147ce87...185887d. Read the comment docs.

Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Thx for this! some minor comments

* @return partitioned SCollections in a `Map`
* @group collection
*/
def partitionToMap[U: Coder](partitionKeys: Set[U], f: T => U): Map[U, SCollection[T]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to partition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is what I wanted to do originally but the compiler didn't like it.

Caused errors for example in the partition by predicate method it didn't know if to use my new one or the original method. Can that be avoided without breaking backwards compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it will require type ascription, it's ok to add it in this version since already introduces some breaking changes. TBH partition should have been curried

Copy link
Contributor

Choose a reason for hiding this comment

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

on a second thought, it might be better to just use similar function signature to what you had.

def partitionByKey[U: Coder](partitionKeys: Set[U])(f: T => U): Map[U, SCollection[T]] 

@nevillelyh nevillelyh merged commit 6dd8c72 into spotify:master Jun 14, 2019
nevillelyh pushed a commit that referenced this pull request Jun 28, 2019
* Add partitionToMap function

* fix indentation

* addressing review comments

* Addressing review comments, part 2
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

3 participants