[GitHub] [cassandra] blambov commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698

["blambov (via GitHub)" <gi...@apache.org>]

blambov commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093141924


##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -138,6 +144,38 @@ public TypeSerializer<ByteBuffer> getSerializer()
     @Override
     public String toString()
     {
+        if(baseType != null)
+        {
+            return String.format("%s(%s:%s)", getClass().getName(),  partitioner.getClass().getName(), baseType.toString()); 
+        }
         return String.format("%s(%s)", getClass().getName(), partitioner.getClass().getName());
     }
+    
+    public AbstractType<?>  getBaseType() 
+    { 
+        return baseType;
+    }
+
+    @Override
+    public boolean equals(Object obj)
+    {
+        if (this == obj)
+        {
+            return true;
+        }
+        if (obj instanceof PartitionerDefinedOrder)
+        {
+            PartitionerDefinedOrder other = (PartitionerDefinedOrder) obj;
+            if (baseType == null && other.baseType == null)

Review Comment:
   Nit: I would make use of `Objects.equals` to simplify these conditionals to
   ```
   return other != null 
       && Objects.equals(this.partitioner, other.partitioner) 
       && Objects.equals(this.baseType, other.baseType);
   ```



##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -88,7 +93,8 @@ public Term fromJSONObject(Object parsed)
     @Override
     public String toJSONString(ByteBuffer buffer, ProtocolVersion protocolVersion)
     {
-        throw new UnsupportedOperationException();
+        assert baseType != null && !baseType.equals(this) : "PartitionerDefinedOrder's toJSONString method need a baseType but now is null or with a not euqal type.";

Review Comment:
   Nit: The second part of this assertion does not make sense.
   
   There is no way to construct a type for which `baseType == this`, because `baseType` is passed before `this` is constructed.



##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -735,12 +736,18 @@ public static TableMetadata indexCfsMetadata(TableMetadata baseCfsMetadata, Inde
         ColumnMetadata indexedColumn = target.left;
         AbstractType<?> indexedValueType = utils.getIndexedValueType(indexedColumn);
 
+        AbstractType<?> indexedTablePartitionKeyType =  baseCfsMetadata.partitioner.partitionOrdering();

Review Comment:
   Is `IPartitioner.partitionOrdering()` used without being followed with `withBaseType()` anywhere now? 
   
   Since this is no longer a patch that will go in older versions, you can change the `IPartitioner` interface to give the base type as a `partitionOrdering` argument, which will remove the need for the special case below and the casts here as well as in `parse`.



##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -131,6 +138,59 @@ public AbstractType<?> parse() throws SyntaxException, ConfigurationException
             return getAbstractType(name);
     }
 
+    /**
+     * parse PartitionOrdering from old version of PartitionOrdering' string format 
+     * */
+    private static  AbstractType<?> defaultParsePartitionOrdering(TypeParser typeParser)
+    {
+        IPartitioner partitioner = DatabaseDescriptor.getPartitioner();
+        Iterator<String> argIterator = typeParser.getKeyValueParameters().keySet().iterator();
+        if (argIterator.hasNext())
+        {
+            partitioner = FBUtilities.newPartitioner(argIterator.next());
+            assert !argIterator.hasNext();
+        }
+        return partitioner.partitionOrdering();
+    }
+
+    /**
+     * parse and return the real PartitionerDefinedOrder from the string variable str
+     * the str format can be like PartitionerDefinedOrder(<partitioner>) or
+     * PartitionerDefinedOrder(<partitioner>:<baseType>)
+     * */
+    public AbstractType<?> getPartitionerDefinedOrder()
+    {
+        int initIdx = idx;
+        skipBlank();
+        if (isEOS())
+            return defaultParsePartitionOrdering(this);
+        if (str.charAt(idx) != '(')
+            throw new IllegalStateException();
+
+        ++idx; // skipping '('
+        skipBlank();
+
+        String k = readNextIdentifier();
+        IPartitioner partitioner = FBUtilities.newPartitioner(k);
+        skipBlank();
+        if (str.charAt(idx) == ':')
+        {
+            ++idx;
+            skipBlank();
+            // must be PartitionerDefinedOrder 
+            PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) partitioner.partitionOrdering();
+            return tmp.withBaseType(parse());
+        }
+        else if (str.charAt(idx) == ')')
+        {
+            idx = initIdx;
+            // if PartitionerDefinedOrder(<partitioner>) then use the original way of parse partitioner Order
+            // for may exist some place we do not know ? or we can just return partitioner.partitionOrdering() here with not initIdx set 
+            return defaultParsePartitionOrdering(this);

Review Comment:
   Since you are already checking that the identifier is followed by `)`, it makes no sense to redo the parsing as `PartitionerDefinedOrder(Partitioner=ignored)` still won't be accepted. Just do `return partitioner.partitionOrdering()` and delete the method above.



-- 
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: pr-unsubscribe@cassandra.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org