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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
