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

Added aggregate & conditonal readers for parquet #172

Merged
merged 5 commits into from
Nov 6, 2018

Conversation

ajayborra
Copy link
Contributor

Related issues
#56

Describe the proposed solution
Added Aggregate & Conditional Reader API for parquet format

@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #172 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage    86.3%   86.29%   -0.02%     
==========================================
  Files         305      305              
  Lines        9963     9967       +4     
  Branches      339      548     +209     
==========================================
+ Hits         8599     8601       +2     
- Misses       1364     1366       +2
Impacted Files Coverage Δ
...m/salesforce/op/readers/ParquetProductReader.scala 100% <ø> (ø) ⬆️
.../scala/com/salesforce/op/readers/DataReaders.scala 85.71% <100%> (+3.36%) ⬆️
.../salesforce/op/features/FeatureBuilderMacros.scala 0% <0%> (-100%) ⬇️
.../op/features/types/FeatureTypeSparkConverter.scala 99.06% <0%> (+0.93%) ⬆️

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 1e17a59...c743da1. Read the comment docs.

@tovbinm
Copy link
Collaborator

tovbinm commented Nov 5, 2018

@ajayborra please add some tests. thanks.

readPath: Option[String],
key: T => String,
val conditionalParams: ConditionalParams[T]
)extends ParquetProductReader[T](readPath, key) with ConditionalDataReader[T]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please fix scalastyle warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tovbinm Seems like we have are going to have similar logic for testing these readers. Will look at the possibility of extracting these as Table Tests and update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any tests included? Why not?

@tovbinm
Copy link
Collaborator

tovbinm commented Nov 5, 2018

@ajayborra Rachel is working on adding a common spec for all the readers, so dont sweat it too much ;) #83

Copy link
Collaborator

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

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

lgtm (tests are flaky, I am working on fixing those)

@tovbinm tovbinm merged commit 17d58a1 into salesforce:master Nov 6, 2018
@ajayborra ajayborra deleted the ab/parquet_agg_cond_readers branch November 6, 2018 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants