[jira] [Commented] (PHOENIX-6587) Validate explicit pre-split and existing splits on salted tables

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PHOENIX-6587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17517737#comment-17517737 ] 

ASF GitHub Bot commented on PHOENIX-6587:
-----------------------------------------

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?





> Validate explicit pre-split and existing splits on salted tables
> ----------------------------------------------------------------
>
>                 Key: PHOENIX-6587
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-6587
>             Project: Phoenix
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 5.2.0
>            Reporter: Istvan Toth
>            Assignee: Istvan Toth
>            Priority: Minor
>
> Specifying the split points manually on salted tables can break them.
> The easiest thing to do is to disallow it, which ensures that the table is created with the default per bucket presplit.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)