Test Hive Querying Capability #266
Conversation
The HiveJdbcGeneralTest class implements methods from the TestMethods class in order to perform data operations on the samdat1.csv dataset after being loaded into HDFS.
|
Looks good to me! |
|
Actually, we do follow "hadoop" java-style, which is same as Java style, except for 2 spaces line indentation and 4 for continuous lines. Could you please reformat it? |
There was a problem hiding this comment.
Overall: please remove all extra blank lines - it makes it hard to read to code. Consider using logger instead of System.out.println calls.
Please add license header to the source code files; add the data files exclusion to the gradle build (and the legacy maven) builds to avoid breaking RAT check
| @@ -0,0 +1,87 @@ | |||
|
|
|||
There was a problem hiding this comment.
The first two lines are blank - no need to have them.
Instead, please put the ALv2 header, like other java files have
| import org.apache.hadoop.fs.Path; | ||
| import org.junit.Test; | ||
| import org.xml.sax.SAXException; | ||
| //Author: Bharat Modi |
There was a problem hiding this comment.
Author is not allowed in Apache project's code
| import org.junit.Test; | ||
| import org.xml.sax.SAXException; | ||
| //Author: Bharat Modi | ||
|
|
There was a problem hiding this comment.
Same here: extra blank likes, missing license header
| public class HiveJdbcGeneralTest extends TestMethods { | ||
|
|
||
| @Test | ||
| public void testTableCreation() throws SQLException, |
There was a problem hiding this comment.
Formatting for the list of the exceptions look all wrong, please fix
| ClassNotFoundException, InstantiationException, | ||
| IllegalAccessException, IOException, URISyntaxException, | ||
| FileNotFoundException, ParserConfigurationException, SAXException { | ||
| final File f = new File(HiveJdbcGeneralTest.class.getProtectionDomain().getCodeSource().getLocation().getPath()); |
There was a problem hiding this comment.
Length of the lines should be kept under 80, if possible. Please check
"REVIEWING PATCHES" section from https://cwiki.apache.org/confluence/display/BIGTOP/How+to+Contribute for more details.
| String hdfsConnection = propertyValue("hdfs-site.xml", "dfs.namenode.rpc-address"); | ||
| String jdbcConnection = System.getenv("HIVE_JDBC_URL"); | ||
|
|
||
| Connection con = null; |
There was a problem hiding this comment.
you don't need null initializer. Java does it for you.
|
|
||
|
|
||
| while (res.next()) { | ||
|
|
There was a problem hiding this comment.
no need for blank lines between the body of a loop and its first line
| System.out.print(columnValue); | ||
| System.out.println(""); | ||
| } | ||
|
|
| for (int i = 1; i <= columnsNumber; i++) { | ||
| String columnValue = res.getString(i); | ||
| System.out.print(columnValue); | ||
| System.out.println(""); |
There was a problem hiding this comment.
Just use prinlln function above and remove the empty string printing, no?
| for (int q=1; q<=columnsNumber; q++){ | ||
| System.out.print(rsmd.getColumnName(q) +" "); | ||
| } | ||
| System.out.println(); |
There was a problem hiding this comment.
two println() calls could be replaced with a single println("\n\n")
| import org.xml.sax.SAXException; | ||
| public class TestMethods { | ||
|
|
||
| protected static String propertyValue(String propertyFile, String propertyName) throws ParserConfigurationException, SAXException, IOException, URISyntaxException{ |
There was a problem hiding this comment.
Are these methods have to be protected static? Why?
| // System.out.println("\n"+"Printing Results: First 5 Columns"+"\n"); | ||
| // while(res.next()){ | ||
| // System.out.println(res.getString(columnList[0])+" "+res.getString(columnList[1])+" "+res.getString(columnList[2])+" "+res.getString(columnList[3])+" "+res.getString(columnList[4])+"\n"); | ||
| // } |
There was a problem hiding this comment.
Why commented out code left here?
I have changed this file based on the comments made, but wasn't sure if the license was applied correctly. The import spaces were done by java and I've cleared out most of the others.
Attempted to clean up the class from spaces, printlns and added the licesnse. I have kept the println's instead of adding logger because the assertEquals result is the only relevant external output.
Update build.gradle
|
The new changes have updated the license statement and the classes should be properly formatted. |
|
Is there a way to drop the commit of the build.gradle file to allow this to pull correctly? @C0S |
|
Not sure what are you asking... Are you saying the patch doesn't apply anymore? If so, you need to rebase and resolve the conflicts between the current HEAD and your local copy. |
|
The build.gradle file shouldn't have been pushed with the others and its whats causing the conflict as it wasn't up to date with other changes in the project. I'm just trying to see if the patch can still be verified/approved but without that file. |
|
You can do This would redo your last commit |
|
Ah, I see. You can do what Olaf recommended. Alternatively, you can copy correct file into your workspace and then amend your existing commit. |
|
Thanks for the help! I tried that, still seems to be giving me this error when I try to make an update to the file. I've looked at the diff and they are exactly the same except for one line I added which points to the sample data I use. |
|
I am looking at the diff and I see a few issue with it. Second, there's an issue with the 'build.gradle file'. It seems there's a mix of commits off different bases. At any rate, you'd need to squash all the commits into one before we can allow it to the master. Does it make sense? Thanks for your patience - git might be a bit too much some times ;) |
The HiveJdbcGeneralTest class implements methods from the TestMethods class in order to perform data operations on the samdat1.csv dataset after being loaded into HDFS.