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

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

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


##########
phoenix-core/src/main/java/org/apache/phoenix/schema/SaltingUtil.java:
##########
@@ -123,4 +130,29 @@ public static void addRegionStartKeyToScanStartAndStopRows(byte[] startKey, byte
             scan.setStopRow(newStopRow);
         }
     }
+
+    public static void checkTableIsSalted(Admin admin, TableName tableName, int buckets) throws IOException, SQLException {
+        // Heuristics to check if and existing HBase table can be a salted Phoenix table
+        // with the specified number of buckets.
+        // May give false negatives.
+        byte[] maxStartKey = new byte[] {0} ;
+        List<RegionInfo> regionInfos =
+                admin.getRegions(tableName);
+        for (RegionInfo regionInfo : regionInfos) {
+            if (Bytes.compareTo(regionInfo.getStartKey(), maxStartKey) > 0) {
+                maxStartKey = regionInfo.getStartKey();
+            }
+        }
+        byte lastRegionStartSaltKey = maxStartKey[0];
+        if (lastRegionStartSaltKey == (byte) (buckets - 1)) {

Review Comment:
   I like the vision of what you are doing here. It's 100% OK to me to try to catch the obvious case(s) that we can and not try to catch all possibilities for messed up region boundaries + salt.
   
   Is it possible to add an "escape hatch" to this method for an experienced user to give a "I know what I'm doing" override? I can't think of a case where your logic is insufficient, but could envision a scenario where something unexpected happens ;) 



##########
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();
-        }

Review Comment:
   I'm trying to double-check that it's intentional that you dropped throwing this exception
   
   > found that merging the regions will break Phoenix, irrespective of the value of phoenix.query.force.rowkeyorder
   
   Did you mean merging salt split points with user-provided split points (like I think this method was doing) or did you just mean general region merging?



-- 
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