Add input/output formatters for Vertica clustered ADBMS.

• Key: MAPREDUCE-775
• Type: New Feature
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Omer Trajman
• Reporter: Omer Trajman
• Component/s: contrib/vertica
• Fix Version/s: 0.21.0

Description

Add native support for Vertica as an input or output format taking advantage of parallel read and write properties of the DBMS.

On the input side allow for parametrized queries (a la prepared statements) and create a split for each combination of parameters. Also support the parameter list to be generated from a sql statement. For example - return metrics for all dimensions that meet criteria X with one input split for each dimension. Divide the read among any number of hosts in the Vertica cluster.

On the output side, support Vertica streaming load to any number of hosts in the Vertica cluster. Output may be to a different cluster than input.

Also includes Input and Output formatters that support streaming interface.

Code has been tested and run on live systems under 19 and 20. Patch for 21 with new API will be ready end of this week.

Comments

[Owen O'Malley]

I'd suggest putting this in a contrib module rather than the main source tree.

[Omer Trajman]

src/contrib/vertica and package org.apache.hadoop.vertica?

[Aaron Kimball]

+1.

[Owen O'Malley]

+1

[Omer Trajman]

Patch introduces Vertica optimized input and output formatters. Includes unit tests (requires vertica jdbc drivers and database) and example as unit test.

We can provide software and license for anyone wanting to run the unit test. All bindings are reflected so the driver is not required to compile.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12414581/MAPREDUCE-775.patch
against trunk revision 800232.

-1 @author. The patch appears to contain 1 @author tags which the Hadoop community has agreed to not allow in code contributions.

+1 tests included. The patch appears to include 15 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

-1 release audit. The applied patch generated 204 release audit warnings (more than the trunk's current 203 warnings).

-1 core tests. The patch failed core unit tests.

-1 contrib tests. The patch failed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/435/testReport/
Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/435/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/435/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/435/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/435/console

This message is automatically generated.

[Arun C Murthy]

Most likely one of the newer files in the patch is missing the Apache License header...

[Omer Trajman]

SQL test script missing the header, eclipse codegen put @author in and I fat fingered the AllTest. Will fix/retest/repatch ASAP.

[Omer Trajman]

Patch to address reported issues including missing license in sql init script and conditional unit tests that won't fail if there's no jdbc driver.

[Omer Trajman]

take 2

[Omer Trajman]

Fixing issues with new patch. I seem to have replaced the original instead of adding a .N.patch - sorry for the confusion.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12415514/MAPREDUCE-775.patch
against trunk revision 807165.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 16 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/511/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/511/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/511/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/511/console

This message is automatically generated.

[Omer Trajman]

Looks like test failures are existing and not related to the patch. Can anyone more familiar with core validate?

[Aaron Kimball]

Omer,

This looks like a great start. I've read through the patch and believe I understand most of how it works. I don't have any major architectural concerns, but there are a number of style issues that I think should be addressed before this is committed. All of these are outlined below. Comments are listed in the sequential order presented by your patch file.

(As for your question about test failures, build #511 has already been deleted by Hudson, so I can't check that.)

ivy.xml:
Do you actually depend on hsqldb?

Hadoop test classes typically go in the same package as that which they test (e.g., o.a.h.vertica), not a separate package like o.a.h.vertica.tests. This would save you a lot of imports in tests, and allows package-public things to be used for testing. (This applies to all your test classes)

AllTests.java

• The method name setUp() has special meaning in JUnit. Since your setup() method isn't a setUp(), can you change this to something less misleadingly-similar?
• In test description string in suite(), o.a.h.vertica, not o.a.h.sqoop.

• re your "TODO: figure out jdbc jar packaging:" Based on Hadoop source tree style, I recommend creating a src/contrib/vertica/lib directory, check any external jars you need in there, and modify src/contrib/vertica/build.xml to include the jars from that dir on the classpath for building, testing, etc. Since patches are text not binary, you should attach the jar to the JIRA issue separately. Note that adding a jar requires its license be A2-compatible. See
  http://www.apache.org/legal/resolved.html#category-a for a list of licenses which external dependencies may have applied to them.

TestExample.Reduce.setup(): I suggest that AllTests.setup(); go in a static initializer block in TestExample rather than getting called in every Reduce.setup() call. Given that you actually require AllTests.setup() in virtually all your tests, I would suggest creating a VerticaTestCase class that subclasses TestCase, have this class call AllTests.setup() in a static initializer block, and then have all your Test* classes subclass VerticaTestCase instead of TestCase. This way you won't worry about missing a call somewhere.

Also in this same method, why catch Exception e and print its stack trace? If Reduce.setup() fails for an exception, why shouldn't the whole test fail?

TestExample.Reduce.reduce(): Style nit: One-line if statements should still use curly-braces around the "then" clause. See http://java.sun.com/docs/codeconv/html/CodeConventions.doc6.html#449. Hadoop source should follow all Sun Java style conventions except for an indentation width of two spaces.

I don't think TestExample, etc, should have a run() method.

TestVertica.testVerticaRecord(): why are values of DATE, TIME, etc, commented out? Dead code should be removed, not commented out. Also, why catch the IOException and return? Why doesn't this method just throw IOException (and implicitly fail the test)?

In recordTest(), don't use "assert values.equals(new_values)", use JUnit: assertEquals("failure message", values, new_values);

Same with testVerticaSplit(), validateInput(), etc...

VerticaStreamingRecordWriter.java: Please use Java "lowerCamelCase" style for field and variable names, not "under_scores" (see writer_table, copy_stmt, etc. These should be writerTable and copyStmt respectively.)

In the constructor, the RuntimeException description "Vertica Formatter requies a the Vertica jdbc driver" contains a bunch of typos.

close() method: if statement should use curly-brace style described above.

write() method: Materializing record.toString() in LOG.debug() for every call to write is expensive. Consider wrapping this statement in a call to LOG.isDebugEnabled().

VerticaConfiguration: comment above definition of DELIMITER has typos.

(input_query.charAt(input_query.length() - 1) == ';' ... perhaps inputQuery.endsWith(';'); ?

getInputParameters() has meaningless javadoc attributes. See also getInputDelmiter() (which is a typo'd method name), setInputDelimiter(), etc... that all have empty @return attributes.

VerticaInputFormat: DateFormat is not thread-safe. datefmt should not be a static member.

This class also has a lot of under_score field and parameter names.

Can your javadoc comment for optimize() suggest when it is appropriate to call this, vs. when you would be better off not doing so? What's the heuristic a programmer should keep in mind?

This method also contains a lot of commented-out code. Please remove it entirely.

conn.wait(1000); should pull out 1000 into a static final constant, or even better, make it configurable.

VerticaRecordWriter.getValue() has hairy braces in an if..else statement. (You do this in write() as well.)

Also, what happens in the case where writer_table.split() returns a 0-length array? In this same method, can you please add a comment explaining why you're pulling rs.getString(4) and rs.getInt(5)? These seem arbitrary as-written.

VerticaInputSplit.executeQuery() has javadoc typos.

VerticaUtil uses tabs instead of spaces, and includes empty lines with leading whitespace. Various block statements and curly braces also require reformatting here, as well as variable_names.

VerticaRecord constructor has meaningless javadoc attributes.
Also, please obey 80-column limit in this class (as well as elsewhere).

in objectTypes(), why not use else if statements instead of just a series of if statements? You could then drop all the continue statements which make for awkward flow. Also, include a case at the end for unknown type where you throw an exception, rather than misalign the types ArrayList from the values ArrayList.

toSQLString(): Please do not start variable names with underscore. I suggest myDelimiter to differentiate it from delimiter.

Also, are fall-thrus in the case block intentional? If so, please mark this with a comment.

[Omer Trajman]

Aaron - thank you for the excellent and thorough comments. All accepted and fixed.

[Omer Trajman]

Updated patch addressing Aaron's feedback.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12419368/MAPREDUCE-775.2.patch
against trunk revision 813944.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 19 new or modified tests.

-1 patch. The patch command could not apply the patch.

Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/68/console

This message is automatically generated.

[Omer Trajman]

whoops...wrong base dir. replacing with same patch from up level up

[Omer Trajman]

Still v2 patch but created from root dir

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12419380/MAPREDUCE-775.2.patch
against trunk revision 814122.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 19 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

-1 javac. The patch appears to cause tar ant target to fail.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed core unit tests.

-1 contrib tests. The patch failed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/69/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/69/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/69/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/69/console

This message is automatically generated.

[Omer Trajman]

need to fix issue in ivy.xml

[Omer Trajman]

close tag in ivy.xml

[Omer Trajman]

crossing fingers

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12419401/MAPREDUCE-775.3.patch
against trunk revision 814122.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 19 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

-1 release audit. The applied patch generated 221 release audit warnings (more than the trunk's current 220 warnings).

+1 core tests. The patch passed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/70/testReport/
Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/70/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/70/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/70/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/70/console

This message is automatically generated.

[Omer Trajman]

forgot apache license header in new fine

[Omer Trajman]

Fourth time's a charm?

[Hadoop QA]

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12419420/MAPREDUCE-775.4.patch
against trunk revision 814122.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 19 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/71/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/71/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/71/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/71/console

This message is automatically generated.

[Aaron Kimball]

Thanks for making those changes. Looks good; +1.

[dhruba borthakur]

I will commit it later tonight if nobody gets to this one before that time.

[Devaraj Das]

I just committed this. Thanks, Omer!

[Hudson]

Integrated in Hadoop-Mapreduce-trunk-Commit #57 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/57/)
. Add native and streaming support for Vertica as an input or output format taking advantage of parallel read and write properties of the DBMS. Contributed by Omer Trajman.