HAWQ-1461. Improve partition parameters validation for PXF-JDBC plugin #1249
Conversation
pxf/build.gradle
Outdated
| @@ -477,7 +481,7 @@ def buildNumber() { | |||
| } | |||
|
|
|||
| task wrapper(type: Wrapper) { | |||
| gradleVersion = '2.11' | |||
| gradleVersion = '2.14.1' | |||
There was a problem hiding this comment.
Why do we need to update Gradle version?
There was a problem hiding this comment.
We are currently using 2 different versions 2.11 and 2.13 (pxf/gradle/wrapper/gradle-wrapper.properties)
| */ | ||
| public void retrieveStats() throws Exception; | ||
|
|
||
| /** | ||
| * Returns next tuple based on statistics information without actual reading of data | ||
| * @return only one row |
There was a problem hiding this comment.
@return next row without reading it from disk
| try { | ||
| range = inConf.getUserProperty("RANGE").split(":"); | ||
| if (inConf.getUserProperty("RANGE") != null) { | ||
| range = inConf.getUserProperty("RANGE").split(":"); |
There was a problem hiding this comment.
calling same function twice, would be better to call it once saving result and then using the result for further logic
| try { | ||
| range = inConf.getUserProperty("RANGE").split(":"); | ||
| if (inConf.getUserProperty("RANGE") != null) { |
There was a problem hiding this comment.
"RANGE" should be an enum value or at least a constant somewhere
| if (inConf.getUserProperty("RANGE") != null) { | ||
| range = inConf.getUserProperty("RANGE").split(":"); | ||
| if (range.length == 1 && partitionType != PartitionType.ENUM) | ||
| throw new UserDataException("The parameter 'RANGE' does not specify '[:end_value]'"); |
There was a problem hiding this comment.
why did we drop support for open-ended range ?
There was a problem hiding this comment.
If we want to support open-ended range on fragmenter phase - we might have the last fragment having more data than rest of fragments, which is acceptable and very user-friendly so +1 for having open-ended range.
There was a problem hiding this comment.
in the similar fashion, we might support open ended lower range as well -- in cases where min value is not known or changes dynamically.
There was a problem hiding this comment.
Supporting open-ended range would require changes to the JDBC code. The current implementation does not support it.
| @@ -144,7 +153,7 @@ public JdbcPartitionFragmenter(InputData inConf) throws UserDataException { | |||
| rangeEnd.setTime(df.parse(range[1])); | |||
| } | |||
| } catch (ParseException e) { | |||
| throw new UserDataException("The parameter 'RANGE' include invalid date format."); | |||
| throw new UserDataException("The parameter 'RANGE' has invalid date format."); | |||
There was a problem hiding this comment.
should we also say what expected format is, such as : "yyyy-MM-dd" ?
| @@ -164,8 +163,8 @@ public void inValidParameterFormat() throws Exception { | |||
| when(inputData.getUserProperty("RANGE")).thenReturn("100:200"); | |||
| try { | |||
| new JdbcPartitionFragmenter(inputData); | |||
| fail("Expected an ArrayIndexOutOfBoundsException"); | |||
| } catch (ArrayIndexOutOfBoundsException ex) { | |||
| fail("Expected an UserDataException"); | |||
There was a problem hiding this comment.
use @test(expected = UserDataException.class) so that there is no need for try / catch / fail lines : https://github.com/junit-team/junit4/wiki/exception-testing
|
|
||
| JdbcPartitionFragmenter fragment = new JdbcPartitionFragmenter(inputData); | ||
| List<Fragment> fragments = fragment.getFragments(); | ||
| assertEquals(fragments.size(), 11); |
There was a problem hiding this comment.
the order of params for assertEquals usually goes (expected, actual) : http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.Object, java.lang.Object) as it will report errors as such, so it might be confusing when error happens what was actually expected and what was returned.
pxf/build.gradle
Outdated
| @@ -111,6 +111,10 @@ subprojects { subProject -> | |||
| force 'org.codehaus.jackson:jackson-mapper-asl:1.9.13' | |||
| force 'junit:junit:4.11' | |||
| } | |||
| configurations { | |||
| exclude group: 'org.json', module: 'json' | |||
There was a problem hiding this comment.
are we sure it will run OK without it ?
No description provided.