You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Maxwell-Guo (via GitHub)" <gi...@apache.org> on 2023/01/30 05:14:08 UTC
[GitHub] [cassandra] Maxwell-Guo opened a new pull request, #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Maxwell-Guo opened a new pull request, #2118:
URL: https://github.com/apache/cassandra/pull/2118
sstabledump errors when dumping data from index for CASSANDRA-17698
```
patch by <maxwellguo>; reviewed by <Reviewers> for CASSANDRA-17698
```
The [Cassandra Jira](https://issues.apache.org/jira/browse/CASSANDRA-17698)
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093941047
##########
src/java/org/apache/cassandra/dht/Murmur3Partitioner.java:
##########
@@ -416,9 +416,9 @@ public Token getMaximumToken()
return new LongToken(Long.MAX_VALUE);
}
- public AbstractType<?> partitionOrdering()
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType)
{
- return partitionOrdering;
+ return ((PartitionerDefinedOrder)partitionOrdering).withBaseType(baseType);
Review Comment:
you are right
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093585505
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
I'm not sure whether the partitioner can be considered a public API, given that one can supply a custom class. If this is so I understand that we should get consensus on the mail list to change its API, even in a major (and I think we are officially in 4.2).
A workaround to keep the API compatibility, if needed, would be providing a default implementation of the new `partitionOrdering(AbstractType)`, and keeping the previous `partitionOrdering()` method.
@blambov wdyt?
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093597111
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
I think it wouldn't, since we wouldn't be breaking any public API compatibility. For context on the discussions about releasing 5.0 and API changes, see:
- https://www.mail-archive.com/dev@cassandra.apache.org/msg19516.html
- https://www.mail-archive.com/dev@cassandra.apache.org/msg19925.html
- https://issues.apache.org/jira/browse/CASSANDRA-17973
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093074349
##########
src/java/org/apache/cassandra/db/marshal/AbstractCompositeType.java:
##########
@@ -266,6 +266,7 @@ public Term fromJSONObject(Object parsed)
@Override
public String toJSONString(ByteBuffer buffer, ProtocolVersion protocolVersion)
{
+ //TODO suport toJSONString for AbstractCompositeType
Review Comment:
```suggestion
// TODO: suport toJSONString (CASSANDRA-18177)
```
##########
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:
```suggestion
assert baseType != null && !baseType.equals(this)
: "PartitionerDefinedOrder's toJSONString method needs a baseType but now it is null or with a not equals type.";
```
##########
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());
Review Comment:
```suggestion
return String.format("%s(%s:%s)", getClass().getName(), partitioner.getClass().getName(), baseType);
```
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -138,6 +144,38 @@ public TypeSerializer<ByteBuffer> getSerializer()
@Override
public String toString()
{
+ if(baseType != null)
Review Comment:
```suggestion
if (baseType != null)
```
##########
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)
Review Comment:
```suggestion
private static AbstractType<?> defaultParsePartitionOrdering(TypeParser typeParser)
```
##########
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:
```suggestion
AbstractType<?> indexedTablePartitionKeyType = baseCfsMetadata.partitioner.partitionOrdering();
```
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -167,14 +227,14 @@ else if (str.charAt(idx) != ',' && str.charAt(idx) != ')')
}
throw new SyntaxException(String.format("Syntax error parsing '%s' at char %d: unexpected end of string", str, idx));
}
-
+
Review Comment:
```suggestion
```
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -18,45 +18,50 @@
package org.apache.cassandra.db.marshal;
import java.nio.ByteBuffer;
-import java.util.Iterator;
-import org.apache.cassandra.config.DatabaseDescriptor;
import org.apache.cassandra.cql3.Term;
import org.apache.cassandra.db.DecoratedKey;
import org.apache.cassandra.db.PartitionPosition;
-import org.apache.cassandra.serializers.TypeSerializer;
-import org.apache.cassandra.serializers.MarshalException;
import org.apache.cassandra.dht.IPartitioner;
+import org.apache.cassandra.serializers.MarshalException;
+import org.apache.cassandra.serializers.TypeSerializer;
import org.apache.cassandra.transport.ProtocolVersion;
import org.apache.cassandra.utils.bytecomparable.ByteComparable;
import org.apache.cassandra.utils.bytecomparable.ByteComparable.Version;
import org.apache.cassandra.utils.bytecomparable.ByteSource;
-import org.apache.cassandra.utils.FBUtilities;
+
/** 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();
+ return parser.getPartitionerDefinedOrder();
}
+ public AbstractType<?> withBaseType(AbstractType<?> baseType)
+ {
+ return new PartitionerDefinedOrder(this.partitioner, baseType);
Review Comment:
```suggestion
return new PartitionerDefinedOrder(partitioner, baseType);
```
##########
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();
+ if(indexedTablePartitionKeyType instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder)indexedTablePartitionKeyType;
Review Comment:
```suggestion
PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) indexedTablePartitionKeyType;
```
##########
src/java/org/apache/cassandra/index/internal/composites/RegularColumnIndex.java:
##########
@@ -76,7 +76,7 @@ public <T> CBuilder buildIndexClusteringPrefix(ByteBuffer partitionKey,
// base table partition should be returned for any mathching index entry.
return builder;
}
-
+
Review Comment:
```suggestion
```
##########
src/java/org/apache/cassandra/tools/Util.java:
##########
@@ -335,6 +338,15 @@ public static TableMetadata metadataFromSSTable(Descriptor desc) throws IOExcept
{
builder.addClusteringColumn("clustering" + (i > 0 ? i : ""), header.getClusteringTypes().get(i));
}
+ if (SecondaryIndexManager.isIndexColumnFamily(desc.cfname))
+ {
+ String index = SecondaryIndexManager.getIndexName(desc.cfname);
+ // Just set the Kind of index to CUSTOM, which is an irrelevant parameter that does't make any effect on the result
Review Comment:
```suggestion
// Just set the Kind of index to CUSTOM, which is an irrelevant parameter that doesn't make any effect on the result
```
##########
test/unit/org/apache/cassandra/SchemaLoader.java:
##########
@@ -390,7 +390,7 @@ public static TableMetadata.Builder staticCFMD(String ksName, String cfName)
.addStaticColumn("val", AsciiType.instance)
.addRegularColumn("val2", AsciiType.instance);
}
-
+
Review Comment:
```suggestion
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
+ differentBaseTypeValidation(new TupleType(Lists.newArrayList(Int32Type.instance, UTF8Type.instance)));
+ // PartitionerDefinedOrder's base type is ReversedType
+ differentBaseTypeValidation(ReversedType.getInstance(Int32Type.instance));
+ // PartitionerDefinedOrder's base type is CollectionType
Review Comment:
```suggestion
// PartitionerDefinedOrder's base type is CollectionType
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
Review Comment:
```suggestion
// PartitionerDefinedOrder's base type is tuple type
```
##########
test/unit/org/apache/cassandra/SchemaLoader.java:
##########
@@ -252,7 +252,7 @@ public static void schemaDefinition(String testName) throws ConfigurationExcepti
if (Boolean.parseBoolean(System.getProperty("cassandra.test.compression", "false")))
useCompression(schema, compressionParams(CompressionParams.DEFAULT_CHUNK_LENGTH));
}
-
+
Review Comment:
```suggestion
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
+ differentBaseTypeValidation(new TupleType(Lists.newArrayList(Int32Type.instance, UTF8Type.instance)));
+ // PartitionerDefinedOrder's base type is ReversedType
+ differentBaseTypeValidation(ReversedType.getInstance(Int32Type.instance));
+ // PartitionerDefinedOrder's base type is CollectionType
+ differentBaseTypeValidation(MapType.getInstance(Int32Type.instance, UTF8Type.instance, false));
+ }
+
+ @Test
+ public void testParsePartitionerOrderMistMatch()
+ {
+ 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;
+ type = tmp.withBaseType(Int32Type.instance);
+ boolean result = partitioner.partitionOrdering().equals(TypeParser.parse(type.toString()));
+ assertFalse(result);
+ }
+ else
+ {
+ // ByteOrderedPartitioner.instance and OrderPreservingPartitioner.instance's partitionOrdering will not be PartitionerDefinedOrder
+ boolean result = partitioner.partitionOrdering().equals(TypeParser.parse(type.toString()));
+ assertTrue(result);
+ }
+ }
+
+ assertEquals(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+ }
+
+ @Test
+ public void testParsePartitionerOrderWithErrorFormat()
+ {
+ for (IPartitioner partitioner: new IPartitioner[] { Murmur3Partitioner.instance,
+ ByteOrderedPartitioner.instance,
+ RandomPartitioner.instance,
+ OrderPreservingPartitioner.instance })
+ {
+ AbstractType<?> type = partitioner.partitionOrdering();
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ // only Murmur3Partitioner and RandomPartitioner's partitionOrdering() are instanceof PartitionerDefinedOrder
+ String msgPartitioner = partitioner instanceof Murmur3Partitioner ? "Murmur3Partitioner" : "RandomPartitioner";
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner,
+ String tmpStr1 = type.toString().replace(')', ',');
+ try
+ {
+ TypeParser.parse(tmpStr1);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Syntax error parsing 'org.apache.cassandra.db.marshal.PartitionerDefinedOrder(org.apache.cassandra.dht." + msgPartitioner + ",: for msg unexpected character ','"));
+ }
+
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner>
+ String tmpStr2 = type.toString().replace(')', '>');
+ try
+ {
+ TypeParser.parse(tmpStr2);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Syntax error parsing 'org.apache.cassandra.db.marshal.PartitionerDefinedOrder(org.apache.cassandra.dht." + msgPartitioner + ">: for msg unexpected character '>'"));
+ }
+
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner>
+ String tmpStr3 = type.toString().replace(')', ':');
+ try
+ {
+ TypeParser.parse(tmpStr3);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Unable to find abstract-type class 'org.apache.cassandra.db.marshal.'"));
+ }
+ }
+ }
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+ }
+
+ private void differentBaseTypeValidation(AbstractType baseType)
Review Comment:
```suggestion
private void differentBaseTypeValidation(AbstractType<?> baseType)
```
##########
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)
+ {
+ return this.partitioner.equals(other.partitioner);
+ }
+ else if (baseType != null && other.baseType != null)
+ {
+ return this.baseType.equals(other.baseType) && this.partitioner.equals(other.partitioner);
+ }
+ return false;
Review Comment:
```suggestion
return partitioner.equals(other.partitioner) && Objects.equals(baseType, other.baseType);
```
##########
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>)
+ * */
Review Comment:
```suggestion
* Parse and return the real {@link PartitionerDefinedOrder} from the string variable {@link #str}.
* The {@link #str} format can be like {@code PartitionerDefinedOrder(<partitioner>)} or
* {@code PartitionerDefinedOrder(<partitioner>:<baseType>)}.
*/
```
##########
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()
Review Comment:
```suggestion
@Nullable
public AbstractType<?> getBaseType()
```
##########
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();
+ if(indexedTablePartitionKeyType instanceof PartitionerDefinedOrder)
Review Comment:
```suggestion
if (indexedTablePartitionKeyType instanceof PartitionerDefinedOrder)
```
##########
src/java/org/apache/cassandra/index/internal/CassandraIndexFunctions.java:
##########
@@ -183,6 +212,22 @@ public TableMetadata.Builder addIndexClusteringColumns(TableMetadata.Builder bui
builder.addClusteringColumn("cell_path", ((CollectionType)columnDef.type).nameComparator());
return builder;
}
+
+ @Override
+ public AbstractType<?> getIndexedPartitionKeyType(ColumnMetadata indexedColumn)
+ {
+ assert indexedColumn.type.isCollection() ;
+ switch (((CollectionType<?>)indexedColumn.type).kind)
+ {
+ case LIST:
+ return ((ListType<?>)indexedColumn.type).getElementsType();
+ case SET:
+ return ((SetType<?>)indexedColumn.type).getElementsType();
+ case MAP:
+ return ((MapType<?, ?>)indexedColumn.type).getValuesType();
Review Comment:
```suggestion
switch (((CollectionType<?>) indexedColumn.type).kind)
{
case LIST:
return ((ListType<?>) indexedColumn.type).getElementsType();
case SET:
return ((SetType<?>) indexedColumn.type).getElementsType();
case MAP:
return ((MapType<?, ?>) indexedColumn.type).getValuesType();
```
##########
src/java/org/apache/cassandra/tools/SSTableExport.java:
##########
@@ -71,8 +72,15 @@
static
{
- DatabaseDescriptor.clientInitialization();
-
+ if (Boolean.getBoolean(Util.ALLOW_TOOL_REINIT_FOR_TEST))
+ {
+ DatabaseDescriptor.clientInitialization(false);//Necessary for testing
+ }
+ else
+ {
+ DatabaseDescriptor.clientInitialization();
+ }
Review Comment:
```suggestion
DatabaseDescriptor.clientInitialization(!Boolean.getBoolean(Util.ALLOW_TOOL_REINIT_FOR_TEST));
```
##########
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
+ * */
Review Comment:
```suggestion
* Parse PartitionOrdering from old version of PartitionOrdering' string format
*/
```
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -167,14 +227,14 @@ else if (str.charAt(idx) != ',' && str.charAt(idx) != ')')
}
throw new SyntaxException(String.format("Syntax error parsing '%s' at char %d: unexpected end of string", str, idx));
}
-
+
public List<AbstractType<?>> getTypeParameters() throws SyntaxException, ConfigurationException
{
List<AbstractType<?>> list = new ArrayList<>();
if (isEOS())
return list;
-
+
Review Comment:
```suggestion
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
Review Comment:
```suggestion
// PartitionerDefinedOrder's base type is composite type
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
+ differentBaseTypeValidation(new TupleType(Lists.newArrayList(Int32Type.instance, UTF8Type.instance)));
+ // PartitionerDefinedOrder's base type is ReversedType
+ differentBaseTypeValidation(ReversedType.getInstance(Int32Type.instance));
+ // PartitionerDefinedOrder's base type is CollectionType
+ differentBaseTypeValidation(MapType.getInstance(Int32Type.instance, UTF8Type.instance, false));
+ }
+
+ @Test
+ public void testParsePartitionerOrderMistMatch()
+ {
+ for (IPartitioner partitioner: new IPartitioner[] { Murmur3Partitioner.instance,
+ ByteOrderedPartitioner.instance,
+ RandomPartitioner.instance,
+ OrderPreservingPartitioner.instance })
Review Comment:
This block is repeated four times in this class, we might want to encapsulate it into a method:
```java
private static void assertForEachPartitioner(Consumer<IPartitioner> consumer)
{
for (IPartitioner partitioner : new IPartitioner[] { Murmur3Partitioner.instance,
ByteOrderedPartitioner.instance,
RandomPartitioner.instance,
OrderPreservingPartitioner.instance })
{
consumer.accept(partitioner);
}
}
```
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -105,4 +109,119 @@ public void testParsePartitionerOrder() throws ConfigurationException, SyntaxExc
}
assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
}
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertSame(DatabaseDescriptor.getPartitioner().partitionOrdering(), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
+ differentBaseTypeValidation(new TupleType(Lists.newArrayList(Int32Type.instance, UTF8Type.instance)));
+ // PartitionerDefinedOrder's base type is ReversedType
Review Comment:
```suggestion
// PartitionerDefinedOrder's base type is ReversedType
```
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093959626
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
AbstractType<?> partitionOrdering();
I will just mark this method Deprecated and left the comment, but I will not remove the public flag of the method , though the public is useless.
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093497565
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -45,6 +45,7 @@
import org.apache.cassandra.db.lifecycle.View;
import org.apache.cassandra.db.marshal.AbstractType;
import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.PartitionerDefinedOrder;
Review Comment:
This and a couple other unused imports are breaking the build
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -131,6 +138,56 @@ 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(null);
+ }
+
+ /**
+ * Parse and return the real {@link PartitionerDefinedOrder} from the string variable {@link #str}.
+ * The {@link #str} format can be like {@code PartitionerDefinedOrder(<partitioner>)} or
+ * {@code PartitionerDefinedOrder(<partitioner>:<baseType>)}.
+ * */
Review Comment:
Same as before, this should be `*/`:
```suggestion
*/
```
##########
test/unit/org/apache/cassandra/dht/LengthPartitioner.java:
##########
@@ -175,8 +175,8 @@ public AbstractType<?> getTokenValidator()
return IntegerType.instance;
}
- public AbstractType<?> partitionOrdering()
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType)
{
- return new PartitionerDefinedOrder(this);
+ return new PartitionerDefinedOrder(this).withBaseType(baseType);
Review Comment:
Nit: could be `new PartitionerDefinedOrder(this, baseType)`, since we publicly expose that constructor overload.
##########
test/unit/org/apache/cassandra/db/marshal/TypeParserTest.java:
##########
@@ -95,14 +101,123 @@ public void testParseError()
@Test
public void testParsePartitionerOrder() throws ConfigurationException, SyntaxException
{
- for (IPartitioner partitioner: new IPartitioner[] { Murmur3Partitioner.instance,
- ByteOrderedPartitioner.instance,
- RandomPartitioner.instance,
- OrderPreservingPartitioner.instance })
+ assertForEachPartitioner(partitioner -> {
+ AbstractType<?> type = partitioner.partitionOrdering(null);
+ assertEquals(type, TypeParser.parse(type.toString()));
+ });
+ assertEquals(DatabaseDescriptor.getPartitioner().partitionOrdering(null), TypeParser.parse("PartitionerDefinedOrder"));
+ }
+
+ @Test
+ public void testParsePartitionerOrderWithBaseType()
+ {
+ // default partitioner
+ assertEquals(DatabaseDescriptor.getPartitioner().partitionOrdering(null), TypeParser.parse("PartitionerDefinedOrder"));
+
+ // PartitionerDefinedOrder's base type is not composite type
+ differentBaseTypeValidation(Int32Type.instance);
+ // PartitionerDefinedOrder's base type is composite type
+ differentBaseTypeValidation(CompositeType.getInstance(Int32Type.instance, UTF8Type.instance));
+ // PartitionerDefinedOrder's base type is tuple type
+ differentBaseTypeValidation(new TupleType(Lists.newArrayList(Int32Type.instance, UTF8Type.instance)));
+ // PartitionerDefinedOrder's base type is ReversedType
+ differentBaseTypeValidation(ReversedType.getInstance(Int32Type.instance));
+ // PartitionerDefinedOrder's base type is CollectionType
+ differentBaseTypeValidation(MapType.getInstance(Int32Type.instance, UTF8Type.instance, false));
+ }
+
+ @Test
+ public void testParsePartitionerOrderMistMatch()
+ {
+ assertForEachPartitioner(partitioner -> {
+ AbstractType<?> type = partitioner.partitionOrdering(null);
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) type;
+ type = tmp.withBaseType(Int32Type.instance);
+ boolean result = partitioner.partitionOrdering(null).equals(TypeParser.parse(type.toString()));
+ assertFalse(result);
+ }
+ else
+ {
+ // ByteOrderedPartitioner.instance and OrderPreservingPartitioner.instance's partitionOrdering will not be PartitionerDefinedOrder
+ boolean result = partitioner.partitionOrdering(null).equals(TypeParser.parse(type.toString()));
+ assertTrue(result);
+ }
+ });
+ assertEquals(DatabaseDescriptor.getPartitioner().partitionOrdering(null), TypeParser.parse("PartitionerDefinedOrder"));
+ }
+
+ @Test
+ public void testParsePartitionerOrderWithErrorFormat()
+ {
+ assertForEachPartitioner(partitioner -> {
+ AbstractType<?> type = partitioner.partitionOrdering(null);
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ // only Murmur3Partitioner and RandomPartitioner's partitionOrdering() are instanceof PartitionerDefinedOrder
+ String msgPartitioner = partitioner instanceof Murmur3Partitioner ? "Murmur3Partitioner" : "RandomPartitioner";
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner,
+ String tmpStr1 = type.toString().replace(')', ',');
+ try
+ {
+ TypeParser.parse(tmpStr1);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Syntax error parsing 'org.apache.cassandra.db.marshal.PartitionerDefinedOrder(org.apache.cassandra.dht." + msgPartitioner + ",: for msg unexpected character ','"));
+ }
+
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner>
+ String tmpStr2 = type.toString().replace(')', '>');
+ try
+ {
+ TypeParser.parse(tmpStr2);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Syntax error parsing 'org.apache.cassandra.db.marshal.PartitionerDefinedOrder(org.apache.cassandra.dht." + msgPartitioner + ">: for msg unexpected character '>'"));
+ }
+
+ // error format PartitionerDefinedOrder(org.apache.cassandra.dht.Murmur3Partitioner>
+ String tmpStr3 = type.toString().replace(')', ':');
+ try
+ {
+ TypeParser.parse(tmpStr3);
+ fail();
+ }
+ catch (Throwable t)
+ {
+ assertTrue(t.getCause().getMessage().contains("Unable to find abstract-type class 'org.apache.cassandra.db.marshal.'"));
+ }
+ }
+ });
+ assertEquals(DatabaseDescriptor.getPartitioner().partitionOrdering(null), TypeParser.parse("PartitionerDefinedOrder"));
+ }
+
+ private void differentBaseTypeValidation(AbstractType<?> baseType)
+ {
+ assertForEachPartitioner(partitioner -> {
+ AbstractType<?> type = partitioner.partitionOrdering(null);
+ if (type instanceof PartitionerDefinedOrder)
+ {
+ PartitionerDefinedOrder tmp = (PartitionerDefinedOrder) type;
+ type = tmp.withBaseType(baseType);
+ }
+ assertEquals(type, TypeParser.parse(type.toString()));
Review Comment:
Changing this from `assertSame` to `assertEquals` has left one of the unused imports that break the build.
##########
test/unit/org/apache/cassandra/tools/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.tools;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final ObjectMapper mapper = new ObjectMapper();
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ System.setProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST, "true"); // Necessary for testing
+ }
+
+ @AfterClass
+ public static void afterClass()
+ {
+ System.clearProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST);
+ }
+
+ @Test
+ public void testRegularColumnIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int PRIMARY KEY, v int)";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
Review Comment:
This file has multiple `for` loops without a whitespace before the parenthesis:
```suggestion
for (ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
```
You can find the codestyle for the project here: https://cassandra.apache.org/_/development/code_style.html
##########
test/unit/org/apache/cassandra/db/marshal/PartitionerDefinedOrderTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.marshal;
+
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import org.apache.cassandra.dht.ByteOrderedPartitioner;
+import org.apache.cassandra.dht.IPartitioner;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.dht.OrderPreservingPartitioner;
+import org.apache.cassandra.dht.RandomPartitioner;
Review Comment:
This and a couple other unused imports are breaking the build
##########
src/java/org/apache/cassandra/db/marshal/TypeParser.java:
##########
@@ -131,6 +138,56 @@ public AbstractType<?> parse() throws SyntaxException, ConfigurationException
return getAbstractType(name);
}
+ /**
+ * Parse PartitionOrdering from old version of PartitionOrdering' string format
+ * */
Review Comment:
Same as in the review for 3.0 and my previous round for trunk: this should be `*/`:
```suggestion
*/
```
##########
src/java/org/apache/cassandra/dht/Murmur3Partitioner.java:
##########
@@ -416,9 +416,9 @@ public Token getMaximumToken()
return new LongToken(Long.MAX_VALUE);
}
- public AbstractType<?> partitionOrdering()
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType)
{
- return partitionOrdering;
+ return ((PartitionerDefinedOrder)partitionOrdering).withBaseType(baseType);
Review Comment:
Why not make the type `Murmur3Partitioner#partitionOrdering` a `PartitionerDefinedOrder`, so we don't have to cast it here?
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
A few nits:
- The `public` keyword is redundant
- The argument can be `null`
- The argument could be named in a more specific way (updating the JavaDoc comment, including a mention of when it's expected to be `null`)
```suggestion
AbstractType<?> partitionOrdering(@Nullable AbstractType<?> partitionKeyType);
```
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -88,7 +95,8 @@ public Term fromJSONObject(Object parsed)
@Override
public String toJSONString(ByteBuffer buffer, ProtocolVersion protocolVersion)
{
- throw new UnsupportedOperationException();
+ assert baseType != null : "PartitionerDefinedOrder's toJSONString method need a baseType but now is null .";
Review Comment:
```suggestion
assert baseType != null : "PartitionerDefinedOrder's toJSONString method needs a base type but now is null.";
```
##########
test/unit/org/apache/cassandra/tools/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.tools;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final ObjectMapper mapper = new ObjectMapper();
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ System.setProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST, "true"); // Necessary for testing
+ }
+
+ @AfterClass
+ public static void afterClass()
+ {
+ System.clearProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST);
+ }
+
+ @Test
+ public void testRegularColumnIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int PRIMARY KEY, v int)";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testPartitionKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int, v int, c text, primary key((k, v)))";
+ String createIndex = "CREATE INDEX ON %s (k)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testKeysWithStaticIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, primary key(k, v))";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v, s) VALUES (0, 0, 's')";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testClusteringIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (c)";
+ String insert = "INSERT INTO %s (k, v, s, c) VALUES (0, 0, 's', 10)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+
+ @Test
+ public void testCollectionMapKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (KEYS(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testCollectionMapValueIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (VALUES(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testCollectionListIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (l)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testCollectionSetIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (st)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ private Pair<String, String> generateSstable(String createTableCql, String createIndexCql, String insertCql) throws Throwable
+ {
+ String table = createTable(createTableCql);
+ String index = createIndex(createIndexCql);
Review Comment:
```suggestion
String index = createIndex(createIndexCql);
```
##########
test/unit/org/apache/cassandra/tools/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.tools;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final ObjectMapper mapper = new ObjectMapper();
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ System.setProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST, "true"); // Necessary for testing
+ }
+
Review Comment:
There are multiple empty lines with trailing whitespaces in this file.
```suggestion
```
##########
test/unit/org/apache/cassandra/tools/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.tools;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final ObjectMapper mapper = new ObjectMapper();
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ System.setProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST, "true"); // Necessary for testing
+ }
+
+ @AfterClass
+ public static void afterClass()
+ {
+ System.clearProperty(org.apache.cassandra.tools.Util.ALLOW_TOOL_REINIT_FOR_TEST);
+ }
+
+ @Test
+ public void testRegularColumnIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int PRIMARY KEY, v int)";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ }
+
+ @Test
+ public void testPartitionKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int, v int, c text, primary key((k, v)))";
+ String createIndex = "CREATE INDEX ON %s (k)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for(ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ for(SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
Review Comment:
This block of code is repeated verbatim 8 times in this file. I would encapsulate it into a method so the 8 tests using it can be one-liners. That should save us more that one hundred lines of duplicated code:
```java
@Test
public void testCollectionSetIndex() throws Throwable
{
testIndex("CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))",
"CREATE INDEX ON %s (st)",
"INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})");
}
private void testIndex(String createTable, String createIndex, String insert) throws Throwable
{
Pair<String, String> tableIndex = generateSstable(createTable, createIndex, insert);
ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
assertTrue(cfs.indexManager.hasIndexes());
assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
for (ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
{
assertTrue(columnFamilyStore.isIndex());
for (SSTableReader sst : columnFamilyStore.getLiveSSTables())
{
String file = sst.getFilename();
ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
List<Map<String, Object>> parsed = mapper.readValue(tool.getStdout(), jacksonListOfMapsType);
assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
}
}
}
```
##########
src/java/org/apache/cassandra/dht/RandomPartitioner.java:
##########
@@ -342,9 +342,9 @@ public AbstractType<?> getTokenValidator()
return IntegerType.instance;
}
- public AbstractType<?> partitionOrdering()
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType)
{
- return partitionOrdering;
+ return ((PartitionerDefinedOrder)partitionOrdering).withBaseType(baseType);
Review Comment:
`RandomPartitioner#partitionOrdering` can be declared with type `PartitionerDefinedOrder`, so we don't need to cast here.
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093956238
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
ok
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1230336089
##########
test/unit/org/apache/cassandra/cql3/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.tools.SSTableExport;
+import org.apache.cassandra.tools.ToolRunner;
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.utils.JsonUtils;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+ private static boolean initValue;
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ initValue = TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.getBoolean();
+ TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.setBoolean(true);
+ }
+
+ @AfterClass
+ public static void afterClass()
+ {
+ TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.setBoolean(initValue);
+ }
+
+ @Test
+ public void testRegularColumnIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int PRIMARY KEY, v int)";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testPartitionKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int, v int, c text, primary key((k, v)))";
+ String createIndex = "CREATE INDEX ON %s (k)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testKeysWithStaticIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, primary key(k, v))";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v, s) VALUES (0, 0, 's')";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testClusteringIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (c)";
+ String insert = "INSERT INTO %s (k, v, s, c) VALUES (0, 0, 's', 10)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionMapKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (KEYS(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionMapValueIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (VALUES(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionListIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (l)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionSetIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (st)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ private void indexSstableValidation(String createTableCql, String createIndexCql, String insertCql) throws Throwable
+ {
+ String SUCEESS_MSG = "Test passed";
+ try
+ {
+ Pair<String, String> tableIndex = generateSstable(createTableCql, createIndexCql, insertCql);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for (ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ assertFalse(columnFamilyStore.getLiveSSTables().isEmpty());
+ for (SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = JsonUtils.JSON_OBJECT_MAPPER.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ fail(SUCEESS_MSG);
+ }
+ catch (Throwable throwable)
+ {
+ // UPGRADING or NONE
+ if (DatabaseDescriptor.getStorageCompatibilityMode().isAfter(5))
+ {
+ assertEquals(SUCEESS_MSG, throwable.getMessage());
+ }
+ else
+ {
+ System.out.println(throwable.getMessage());
Review Comment:
ok
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1094352981
##########
src/java/org/apache/cassandra/dht/ByteOrderedPartitioner.java:
##########
@@ -17,6 +17,7 @@
*/
package org.apache.cassandra.dht;
+import org.apache.cassandra.db.marshal.UTF8Type;
Review Comment:
Unneeded change, it breaks the build.
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093224212
##########
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:
So I just should remove this second assertion
--
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
[GitHub] [cassandra] blambov commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093617160
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
That's what I mean with the workaround:
```java
@Deprecated // use #partitionOrdering(AbstractType) instead, see CASSANDRA-17698 for details
AbstractType<?> partitionOrdering();
default AbstractType<?> partitionOrdering(@Nullable AbstractType<?> partitionKeyType)
{
return partitionOrdering();
}
```
And then the proper partitioner implementations can override `partitionOrdering(AbstractType)`.
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1230335958
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -138,6 +147,33 @@ public TypeSerializer<ByteBuffer> getSerializer()
@Override
public String toString()
{
+ if (DatabaseDescriptor.getStorageCompatibilityMode().isAfter(5) &&
Review Comment:
ok, +1 on this.
--
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
[GitHub] [cassandra] blambov commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093587398
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -128,8 +128,9 @@ default Token getMaximumToken()
/**
* Abstract type that orders the same way as DecoratedKeys provided by this partitioner.
* Used by secondary indices.
+ * @param baseType base type for PartitionerDefinedOrder
*/
- public AbstractType<?> partitionOrdering();
+ public AbstractType<?> partitionOrdering(AbstractType<?> baseType);
Review Comment:
> A workaround to keep the API compatibility, if needed, would be providing a default implementation of the new `partitionOrdering(AbstractType)`, and keeping the previous `partitionOrdering()` method.
This works for me. Would it need mailing list agreement too?
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093225924
##########
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:
yes I agree, as ths comment I left 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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1093227906
##########
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:
ok, good point.
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2118:
URL: https://github.com/apache/cassandra/pull/2118#discussion_r1229888532
##########
test/unit/org/apache/cassandra/cql3/SecondaryIndexSSTableExportTest.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.tools.SSTableExport;
+import org.apache.cassandra.tools.ToolRunner;
+import org.apache.cassandra.utils.Pair;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.utils.JsonUtils;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class SecondaryIndexSSTableExportTest extends CQLTester
+{
+ private static final TypeReference<List<Map<String, Object>>> jacksonListOfMapsType = new TypeReference<List<Map<String, Object>>>() {};
+ private static boolean initValue;
+
+ @BeforeClass
+ public static void beforeClass()
+ {
+ initValue = TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.getBoolean();
+ TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.setBoolean(true);
+ }
+
+ @AfterClass
+ public static void afterClass()
+ {
+ TEST_UTIL_ALLOW_TOOL_REINIT_FOR_TEST.setBoolean(initValue);
+ }
+
+ @Test
+ public void testRegularColumnIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int PRIMARY KEY, v int)";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testPartitionKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int, v int, c text, primary key((k, v)))";
+ String createIndex = "CREATE INDEX ON %s (k)";
+ String insert = "INSERT INTO %s (k, v) VALUES (0, 0)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testKeysWithStaticIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, primary key(k, v))";
+ String createIndex = "CREATE INDEX ON %s (v)";
+ String insert = "INSERT INTO %s (k, v, s) VALUES (0, 0, 's')";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testClusteringIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (c)";
+ String insert = "INSERT INTO %s (k, v, s, c) VALUES (0, 0, 's', 10)";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionMapKeyIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (KEYS(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionMapValueIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (VALUES(m))";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionListIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (l)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ @Test
+ public void testCollectionSetIndex() throws Throwable
+ {
+ String createTable = "CREATE TABLE %s (k int , v int, s text static, c bigint, m map<bigint, text>, l list<text>, st set<int>, primary key((k, v), c))";
+ String createIndex = "CREATE INDEX ON %s (st)";
+ String insert = "INSERT INTO %s (k, v, s, c, m, l, st) VALUES (0, 0, 's', 10, {100:'v'}, ['l1', 'l2'], {1, 2, 3})";
+ indexSstableValidation(createTable, createIndex, insert);
+ }
+
+ private void indexSstableValidation(String createTableCql, String createIndexCql, String insertCql) throws Throwable
+ {
+ String SUCEESS_MSG = "Test passed";
+ try
+ {
+ Pair<String, String> tableIndex = generateSstable(createTableCql, createIndexCql, insertCql);
+ ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
+ assertTrue(cfs.indexManager.hasIndexes());
+ assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
+ for (ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
+ {
+ assertTrue(columnFamilyStore.isIndex());
+ assertFalse(columnFamilyStore.getLiveSSTables().isEmpty());
+ for (SSTableReader sst : columnFamilyStore.getLiveSSTables())
+ {
+ String file = sst.getFilename();
+ ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
+ List<Map<String, Object>> parsed = JsonUtils.JSON_OBJECT_MAPPER.readValue(tool.getStdout(), jacksonListOfMapsType);
+ assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
+ assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
+ }
+ }
+ fail(SUCEESS_MSG);
+ }
+ catch (Throwable throwable)
+ {
+ // UPGRADING or NONE
+ if (DatabaseDescriptor.getStorageCompatibilityMode().isAfter(5))
+ {
+ assertEquals(SUCEESS_MSG, throwable.getMessage());
+ }
+ else
+ {
+ System.out.println(throwable.getMessage());
Review Comment:
I think we can check that we are actually throwing the expected message, and mentioning CASSANDRA-18254 since we are not entirely fixing the bug:
```java
Pair<String, String> tableIndex = generateSstable(createTableCql, createIndexCql, insertCql);
ColumnFamilyStore cfs = getColumnFamilyStore(KEYSPACE, tableIndex.left);
assertTrue(cfs.indexManager.hasIndexes());
assertNotNull(cfs.indexManager.getIndexByName(tableIndex.right));
for (ColumnFamilyStore columnFamilyStore : cfs.indexManager.getAllIndexColumnFamilyStores())
{
assertTrue(columnFamilyStore.isIndex());
assertFalse(columnFamilyStore.getLiveSSTables().isEmpty());
for (SSTableReader sst : columnFamilyStore.getLiveSSTables())
{
String file = sst.getFilename();
try
{
ToolRunner.ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, file);
List<Map<String, Object>> parsed = JsonUtils.JSON_OBJECT_MAPPER.readValue(tool.getStdout(), jacksonListOfMapsType);
assertNotNull(tool.getStdout(), parsed.get(0).get("partition"));
assertNotNull(tool.getStdout(), parsed.get(0).get("rows"));
}
catch (AssertionError e)
{
// TODO: CASSANDRA-18254 should provide a workaround for pre-5.0 sstables
assertTrue(DatabaseDescriptor.getStorageCompatibilityMode().isBefore(5));
Assertions.assertThat(e.getMessage())
.contains("PartitionerDefinedOrder's toJSONString method needs a partition key type but now is null.");
}
catch (MismatchedInputException e)
{
// TODO: CASSANDRA-18254 should provide a workaround for pre-5.0 sstables
assertTrue(DatabaseDescriptor.getStorageCompatibilityMode().isBefore(5));
Assertions.assertThat(e.getMessage())
.contains("No content to map due to end-of-input");
}
}
}
```
##########
src/java/org/apache/cassandra/db/marshal/PartitionerDefinedOrder.java:
##########
@@ -138,6 +147,33 @@ public TypeSerializer<ByteBuffer> getSerializer()
@Override
public String toString()
{
+ if (DatabaseDescriptor.getStorageCompatibilityMode().isAfter(5) &&
Review Comment:
We have to assume that in the future there will be a `StorageCompatibilityMode.CASSANDRA_5(5)`. Its meaning will be "We are in Cassandra 6.x, but we remain compatible with Cassandra 5.x". That is, we will require to use the "o*" format introduced by 5.0 instead of whatever fancy new format we use in 6.0.
In case of using that future CASSANDRA_5 compatibility mode, we should use the current fix. However, the current `DatabaseDescriptor.getStorageCompatibilityMode().isAfter(5)` will return false, and we will keep using the old 4.x behaviour in 6.x compatible with 5.x, which seems incorrect.
So I think that here we should just use `!DatabaseDescriptor.getStorageCompatibilityMode().isBefore(5)` to select the new behaviour. Or we can use `DatabaseDescriptor.getStorageCompatibilityMode().isBefore(5)` to select the old behaviour if you want to avoid the negation, whatever you prefer.
Also, although it's not strictly necessary, I would put the check for `partitionKeyType` before the compatibility check. That's because the compatibility check might have some increased performance cost in the future, whereas the null check is obviously trivial.
So I would use something like:
```java
@Override
public String toString()
{
if (partitionKeyType != null && !DatabaseDescriptor.getStorageCompatibilityMode().isBefore(5))
{
return String.format("%s(%s:%s)", getClass().getName(), partitioner.getClass().getName(), partitionKeyType);
}
// if Cassandra's major version is after 5, use the old behaviour
return String.format("%s(%s)", getClass().getName(), partitioner.getClass().getName());
}
```
The same would apply for the checks on `SecondaryIndexSSTableExportTest` and `TypeParserTest`.
--
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
[GitHub] [cassandra] Maxwell-Guo closed pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo closed pull request #2118: sstabledump errors when dumping data from index for CASSANDRA-17698
URL: https://github.com/apache/cassandra/pull/2118
--
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