[GitHub] [phoenix] ankitsinghal commented on a diff in pull request #1353: PHOENIX-6587 Disallow specifying explicit pre-split on salted tables

[GitBox <gi...@apache.org>]

ankitsinghal commented on code in PR #1353:
URL: https://github.com/apache/phoenix/pull/1353#discussion_r844322557


##########
phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java:
##########
@@ -595,25 +594,35 @@ public static boolean isSystemTable(byte[] fullTableName) {
         if (QueryConstants.SYSTEM_SCHEMA_NAME.equals(schemaName)) return true;
         return false;
     }
-    
-    // Given the splits and the rowKeySchema, find out the keys that 
-    public static byte[][] processSplits(byte[][] splits, LinkedHashSet<PColumn> pkColumns, Integer saltBucketNum, boolean defaultRowKeyOrder) throws SQLException {
-        // FIXME: shouldn't this return if splits.length == 0?
-        if (splits == null) return null;
-        // We do not accept user specified splits if the table is salted and we specify defaultRowKeyOrder. In this case,
-        // throw an exception.
-        if (splits.length > 0 && saltBucketNum != null && defaultRowKeyOrder) {
-            throw new SQLExceptionInfo.Builder(SQLExceptionCode.NO_SPLITS_ON_SALTED_TABLE).build().buildException();
-        }
-        // If the splits are not specified and table is salted, pre-split the table. 
-        if (splits.length == 0 && saltBucketNum != null) {
-            splits = SaltingUtil.getSalteByteSplitPoints(saltBucketNum);
-        }
-        byte[][] newSplits = new byte[splits.length][];
+
+    // Given the splits and the rowKeySchema, add the split points based on saltBucketNum, as well
+    // as the ones passed in the parameter, call processSplit on both, and return the union.
+    public static byte[][] processSplits(byte[][] splits, LinkedHashSet<PColumn> pkColumns, Integer saltBucketNum) throws SQLException {
+        if (splits == null) {
+            splits = new byte[0][];
+        }
+
+        TreeSet<byte[]> allSplits = new TreeSet<>(new Bytes.ByteArrayComparator());
+
+        // Add the salted split points, if any
+        if (saltBucketNum != null) {
+            byte[][] saltSplits = SaltingUtil.getSaltedByteSplitPoints(saltBucketNum);
+            for (int i=0; i<saltSplits.length; i++) {
+                allSplits.add(processSplit(saltSplits[i], pkColumns));
+            }
+        }
+
+        //Add the specified split points
         for (int i=0; i<splits.length; i++) {
-            newSplits[i] = processSplit(splits[i], pkColumns); 
+            allSplits.add(processSplit(splits[i], pkColumns));

Review Comment:
   How we are ensuring that the explicit split will always start with the SALT < buckets?



##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java:
##########
@@ -289,8 +289,11 @@ public MetaDataMutationResult getTable(PName tenantId, byte[] schemaBytes, byte[
     }
     
     private static List<HRegionLocation> generateRegionLocations(byte[] physicalName, byte[][] splits) {
-        byte[] startKey = HConstants.EMPTY_START_ROW;
         List<HRegionLocation> regions = Lists.newArrayListWithExpectedSize(splits.length);
+        if (splits.length == 0) {
+            return regions;
+        }
+        byte[] startKey = HConstants.EMPTY_START_ROW;

Review Comment:
   nit:
   ```suggestion
           if (splits.length == 0) {
               return regions;
           }
           List<HRegionLocation> regions = Lists.newArrayListWithExpectedSize(splits.length);
           byte[] startKey = HConstants.EMPTY_START_ROW;
   ```



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableIT.java:
##########
@@ -63,14 +65,43 @@ public void testTableWithInvalidBucketNumber() throws Exception {
     }
 
     @Test
-    public void testTableWithSplit() throws Exception {
-        try {
-            createTestTable(getUrl(), "create table " + generateUniqueName() + " (a_integer integer not null primary key) SALT_BUCKETS = 4",
-                    new byte[][] {{1}, {2,3}, {2,5}, {3}}, null);
+    public void testTableWithExplicitSplit() throws Exception {
+        byte[][] expectedStartKeys =
+        new byte[][] { {},
+            {1, 0, 0, 0, 0},
+            {2, 0, 0, 0, 0},
+            {2, 3, 0, 0, 0},
+            {2, 5, 0, 0, 0},
+            {3, 0, 0, 0, 0}};
+        String tableName = generateUniqueName();
+        createTestTable(getUrl(), "create table " + tableName + " (a_integer integer not null primary key) SALT_BUCKETS = 4",
+                new byte[][] {{1}, {2,3}, {2,5}, {3}}, null);
+        Admin admin = utility.getAdmin();
+        ArrayList<byte[]> startKeys = new ArrayList<>();
+        List<RegionInfo> regionInfos =
+                admin.getRegions(TableName.valueOf(tableName));
+        for (RegionInfo regionInfo : regionInfos) {
+            startKeys.add(regionInfo.getStartKey());
+        }
+        assertTrue(Arrays.deepEquals(expectedStartKeys, startKeys.toArray()));

Review Comment:
   This makes sense, we are ensuring that we never overlap two buckets.



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org