mosche commented on a change in pull request #15848:
URL: https://github.com/apache/beam/pull/15848#discussion_r800629040



##########
File path: 
sdks/java/io/jdbc/src/test/java/org/apache/beam/sdk/io/jdbc/JdbcUtilTest.java
##########
@@ -41,4 +53,126 @@ public void testGetPreparedStatementSetCaller() throws 
Exception {
     String expectedStmt = "INSERT INTO test_table(col1, col2, col3) VALUES(?, 
?, ?)";
     assertEquals(expectedStmt, generatedStmt);
   }
+
+  @Test
+  public void testStringPartitioningWithSingleKeyFn() {
+    JdbcReadWithPartitionsHelper<String> helper =
+        
JdbcUtil.JdbcReadWithPartitionsHelper.getPartitionsHelper(TypeDescriptors.strings());
+    List<KV<String, String>> expectedRanges =

Review comment:
       @pabloem Unfortunately the string partitions are wrong, for values of 
type string (of undefined length) a deterministic next value does not exist: 
`<=  'a'` is simply not the same as `< 'b'`. 
   
   Trying to do this might include values that should be filtered out (e.g. 
`aa` in the example above) and generate wrong results. 
   
   I'm personally still reluctant to support String partitions for already 
mentioned reasons. If you want to move forward with strings I would recommend 
to wrap boundary values in a small wrapper that contains an additional flag to 
specify if the value is inclusive or not. This way you can adjust your queries 
accordingly and treat intermediate partitions different from the very last one.
   
   
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to