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

Pig #16

Merged
merged 29 commits into from
Sep 25, 2015
Merged

Pig #16

merged 29 commits into from
Sep 25, 2015

Conversation

akshaisarma
Copy link
Contributor

No description provided.

@akshaisarma
Copy link
Contributor Author

Code coverage is broken because of Cobertura's issues with Java 8. I will look into alternatives or get a clover license for Open Source.

@yahoocla
Copy link

CLA is valid!

@akshaisarma
Copy link
Contributor Author

Added clover and applied for a license. Coveralls doesn't support clover yet unfortunately but has an issue for this at trautonen/coveralls-maven-plugin#36

@akshaisarma
Copy link
Contributor Author

Adding some unrelated unit tests. Clover reports coverage at 96.3. It was around 60 before. The coveralls number of 99 was not accurate.

@akshaisarma
Copy link
Contributor Author

Getting checks failed because of coverage drop. I added Jacoco instead of cobertura. It works because the actual coverage report of 96% caused the check to fail. I also bound checkstyle and jacoco to the verify and test lifecycles and temporarily added a line coverage requirement of 95% and branch of 90%. I'll add unit tests to get the check to pass. There is one more class that is not tested well - App.

I also have an open source clover license from Atlassian. I find that using clover for local coverage visualization is better. Please reach out to me for the license if you need it.

@akshaisarma
Copy link
Contributor Author

Coverage is now 99.5. I could have hit 100 but would require sacrificing brevity in TypeSystem.Operations.

@@ -38,11 +38,13 @@
import static java.util.Collections.singletonList;

public class Apiary implements Engine {
public static final String HIVE_JDBC = "hive-jdbc";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for pulling all these string constants out. Much cleaner.

@joshwalters
Copy link
Contributor

This looks good to me, awesome stuff!

@akshaisarma
Copy link
Contributor Author

I do want to test against a Hadoop cluster and perhaps write some documentation on what Pig/Hadoop jars and resources are needed to run it. I also want to test if registering UDFs etc in the script will work. I will hold off merging till that is cleared up. We may also need more settings that PigMain sets by default, notably tmpfilecompression etc.

akshaisarma added a commit that referenced this pull request Sep 25, 2015
@akshaisarma akshaisarma merged commit 7dcd8f7 into master Sep 25, 2015
@akshaisarma akshaisarma deleted the pig branch September 25, 2015 16:21
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