You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/11/14 15:47:45 UTC
[GitHub] [cassandra] blambov commented on a diff in pull request #1998: sstabledump errors when dumping data from index for CASSANDRA-17698
blambov commented on code in PR #1998:
URL: https://github.com/apache/cassandra/pull/1998#discussion_r1021677876
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
+ if (str.charAt(idx) != '(')
+ throw new IllegalStateException();
+ Pair<Boolean, AbstractType<?>> result = null;
+ ++idx; // skipping '('
+
+ if (str.charAt(idx) == ')')
+ {
+ ++idx;
+ return defaultParsePartitionOrdering(this);
+ }
+ skipBlank();
Review Comment:
This also makes better sense before the check above.
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
+ if (str.charAt(idx) != '(')
+ throw new IllegalStateException();
+ Pair<Boolean, AbstractType<?>> result = null;
+ ++idx; // skipping '('
+
+ if (str.charAt(idx) == ')')
+ {
+ ++idx;
+ return defaultParsePartitionOrdering(this);
+ }
+ skipBlank();
+ String k = readNextIdentifier();
+ skipBlank();
+ if (str.charAt(idx) == ':')
+ {
+ ++idx;
+ skipBlank();
+ }
+ else if (str.charAt(idx) != ',' && str.charAt(idx) != ')')
Review Comment:
Shouldn't we throw an error if the next part is neither a separator nor a closing bracket?
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -98,4 +98,24 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType() throws ConfigurationException, SyntaxException
+ {
+ for (IPartitioner partitioner: new IPartitioner[] { Murmur3Partitioner.instance,
+ ByteOrderedPartitioner.instance,
+ RandomPartitioner.instance,
+ OrderPreservingPartitioner.instance })
+ {
+ AbstractType<?> type = partitioner.partitionOrdering();
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) type;
+ tmp.withBaseType(Int32Type.instance);
+ }
+ System.out.println(type.toString());
+ assertSame(type, TypeParser.parse(type.toString()));
+ }
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+ }
Review Comment:
It would be helpful to test some explicit definitions to try all acceptable variations (default partitioner, partitioner without base type, partitioner with base type).
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
Review Comment:
I would move this before the `isEOS` check.
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -28,29 +29,38 @@
import org.apache.cassandra.dht.IPartitioner;
import org.apache.cassandra.utils.ByteBufferUtil;
import org.apache.cassandra.utils.FBUtilities;
+import org.apache.cassandra.utils.Pair;
/** for sorting columns representing row keys in the row ordering as determined by a partitioner.
* Not intended for user-defined CFs, and will in fact error out if used with such. */
public class PartitionerDefinedOrder extends AbstractType<ByteBuffer>
{
private final IPartitioner partitioner;
-
+ private final AbstractType<?> baseType;
+
public PartitionerDefinedOrder(IPartitioner partitioner)
{
super(ComparisonType.CUSTOM);
this.partitioner = partitioner;
+ this.baseType = null;
+ }
+
+ public PartitionerDefinedOrder(IPartitioner partitioner, AbstractType<?> baseType)
+ {
+ super(ComparisonType.CUSTOM);
+ this.partitioner = partitioner;
+ this.baseType = baseType;
}
public static AbstractType<?> getInstance(TypeParser parser)
{
- IPartitioner partitioner = DatabaseDescriptor.getPartitioner();
- Iterator<String> argIterator = parser.getKeyValueParameters().keySet().iterator();
- if (argIterator.hasNext())
- {
- partitioner = FBUtilities.newPartitioner(argIterator.next());
- assert !argIterator.hasNext();
- }
- return partitioner.partitionOrdering();
+ TypeParser clone = parser.clone();
+ return clone.getPartitionerDefinedOrder();
Review Comment:
There shouldn't be a need for `clone` here. (The clone might even break something, in the unlikely case of this being part of a composite/tuple.)
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
+ if (str.charAt(idx) != '(')
+ throw new IllegalStateException();
+ Pair<Boolean, AbstractType<?>> result = null;
+ ++idx; // skipping '('
+
+ if (str.charAt(idx) == ')')
+ {
+ ++idx;
+ return defaultParsePartitionOrdering(this);
+ }
+ skipBlank();
+ String k = readNextIdentifier();
+ skipBlank();
+ if (str.charAt(idx) == ':')
+ {
+ ++idx;
+ skipBlank();
+ }
+ else if (str.charAt(idx) != ',' && str.charAt(idx) != ')')
+ {
+ return defaultParsePartitionOrdering(this);
+ }
+ IPartitioner partitioner = FBUtilities.newPartitioner(k);
+ AbstractType<?> type = partitioner.partitionOrdering();
+ if (partitioner.partitionOrdering() instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) partitioner.partitionOrdering();
+ ++idx;
+ try
+ {
+ type = tmp.withBaseType(parse());
+ }
+ catch (Throwable throwable)
+ {
+ Iterator<String> argIterator = this.getKeyValueParameters().keySet().iterator();
Review Comment:
Shouldn't we just throw an error in this case?
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
+ if (str.charAt(idx) != '(')
+ throw new IllegalStateException();
+ Pair<Boolean, AbstractType<?>> result = null;
+ ++idx; // skipping '('
+
+ if (str.charAt(idx) == ')')
+ {
+ ++idx;
+ return defaultParsePartitionOrdering(this);
+ }
+ skipBlank();
+ String k = readNextIdentifier();
+ skipBlank();
+ if (str.charAt(idx) == ':')
+ {
+ ++idx;
+ skipBlank();
+ }
+ else if (str.charAt(idx) != ',' && str.charAt(idx) != ')')
+ {
+ return defaultParsePartitionOrdering(this);
+ }
+ IPartitioner partitioner = FBUtilities.newPartitioner(k);
+ AbstractType<?> type = partitioner.partitionOrdering();
+ if (partitioner.partitionOrdering() instanceof PartitionerDefinedOrder)
Review Comment:
Looks like this test should be checking if a `:` was seen instead.
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
+ public AbstractType<?> getPartitionerDefinedOrder()
+ {
+ if (isEOS())
+ return defaultParsePartitionOrdering(this);
+ skipBlank();
+ if (str.charAt(idx) != '(')
+ throw new IllegalStateException();
+ Pair<Boolean, AbstractType<?>> result = null;
+ ++idx; // skipping '('
+
+ if (str.charAt(idx) == ')')
+ {
+ ++idx;
+ return defaultParsePartitionOrdering(this);
Review Comment:
Couldn't we just use `DatabaseDescriptor.getPartitioner().partitionerOrdering()` here?
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -130,6 +138,73 @@ 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();
+ }
+
+ //the format is (partitioner:type)
Review Comment:
Could you expand on this? The actual format appears to permit more, at least `PartitionerDefinedOrder`, `PartitionerDefinedOrder(<partitioner>)` and `PartitionerDefinedOrder(<partitioner>:<baseType>)`.
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -98,4 +98,24 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType() throws ConfigurationException, SyntaxException
+ {
+ for (IPartitioner partitioner: new IPartitioner[] { Murmur3Partitioner.instance,
+ ByteOrderedPartitioner.instance,
+ RandomPartitioner.instance,
+ OrderPreservingPartitioner.instance })
+ {
+ AbstractType<?> type = partitioner.partitionOrdering();
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) type;
+ tmp.withBaseType(Int32Type.instance);
Review Comment:
I believe this should be `type = ...`.
--
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