You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/05/16 15:00:12 UTC
[GitHub] [cassandra] adelapena commented on a diff in pull request #1621: CASSANDRA-17623 - Fix issue where frozen maps may not be serialized in the correct order
adelapena commented on code in PR #1621:
URL: https://github.com/apache/cassandra/pull/1621#discussion_r873803244
##########
test/unit/org/apache/cassandra/cql3/validation/operations/InsertTest.java:
##########
@@ -247,4 +252,42 @@ public void testCKInsertWithValueOver64K() throws Throwable
assertInvalidThrow(InvalidRequestException.class,
"INSERT INTO %s (a, b) VALUES ('foo', ?)", new String(TOO_BIG.array()));
}
+
+ @Test
+ public void testInsertingMapDataWithParameterizedQueriesIsKeyOrderIndependent() throws Throwable
Review Comment:
This test is mostly identical to the one on `SelectTest`, we could combine them and put them in `CollectionsTest.java`.
##########
test/unit/org/apache/cassandra/cql3/validation/operations/InsertTest.java:
##########
@@ -247,4 +252,42 @@ public void testCKInsertWithValueOver64K() throws Throwable
assertInvalidThrow(InvalidRequestException.class,
"INSERT INTO %s (a, b) VALUES ('foo', ?)", new String(TOO_BIG.array()));
}
+
+ @Test
+ public void testInsertingMapDataWithParameterizedQueriesIsKeyOrderIndependent() throws Throwable
+ {
+ UUID uuid1 = UUIDs.timeBased();
+ UUID uuid2 = UUIDs.timeBased();
+ createTable("CREATE TABLE %s (k text, c frozen<map<timeuuid, text>>, PRIMARY KEY (k, c));");
Review Comment:
The fix is for both maps and sets but the tests only exercise maps. I think we should add equivalent tests for sets. for example:
```java
@Test
public void testInsertingSetDataWithParameterizedQueriesIsKeyOrderIndependent() throws Throwable
{
UUID uuid1 = UUIDs.timeBased();
UUID uuid2 = UUIDs.timeBased();
createTable("CREATE TABLE %s (k text, c frozen<set<timeuuid>>, PRIMARY KEY (k, c));");
// We use `executeNet` in this test because `execute` passes parameters through CqlTester#transformValues(), which calls
// AbstractType#decompose() on the value, which "fixes" the map order, but wouldn't happen normally.
executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", set(uuid1, uuid2));
executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", set(uuid2, uuid1));
beforeAndAfterFlush(() -> {
assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid1 + ", " + uuid2 + '}'), 1);
assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid2 + ", " + uuid1 + '}'), 1);
assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid1 + ", " + uuid2 + '}'), 1);
assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid2 + ", " + uuid1 + '}'), 1);
});
}
@Test
public void testInsertingSetDataWithParameterizedQueriesIsKeyOrderIndependentWithSelectSlice() throws Throwable
{
UUID uuid1 = UUIDs.timeBased();
UUID uuid2 = UUIDs.timeBased();
createTable("CREATE TABLE %s (k text, c frozen<set<timeuuid>>, PRIMARY KEY (k, c));");
// We use `executeNet` in this test because `execute` passes parameters through CqlTester#transformValues(), which calls
// AbstractType#decompose() on the value, which "fixes" the map order, but wouldn't happen normally.
beforeAndAfterFlush(() -> {
executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", set(uuid2, uuid1));
assertRowsNet(executeNet("SELECT k, c[" + uuid2 + "] from %s"), row("0", uuid2));
assertRowsNet(executeNet("SELECT k, c[" + uuid1 + "] from %s"), row("0", uuid1));
});
}
```
##########
test/unit/org/apache/cassandra/cql3/CQLTester.java:
##########
@@ -2351,4 +2359,12 @@ public boolean equals(Object o)
&& Objects.equal(password, u.password);
}
}
+
+ protected static <K,V> Map<K,V> buildInsertionOrderedMap(K k1, V v1, K k2, V v2)
Review Comment:
We can use the already existent `CQLTester#map(Object...)` instead of adding a new method.
##########
test/unit/org/apache/cassandra/io/sstable/CQLSSTableWriterTest.java:
##########
@@ -947,6 +952,180 @@ public void testDateType() throws Exception
}
}
+ @Test
+ public void testFrozenMapType() throws Exception
+ {
+ // Test to make sure we can write to `date` fields in both old and new formats
+ String schema = "CREATE TABLE " + qualifiedTable + " ("
+ + " k text,"
+ + " c frozen<map<text, text>>,"
+ + " PRIMARY KEY (k, c)"
+ + ")";
+ String insert = "INSERT INTO " + qualifiedTable + " (k, c) VALUES (?, ?)";
+ CQLSSTableWriter writer = CQLSSTableWriter.builder()
+ .inDirectory(dataDir)
+ .forTable(schema)
+ .using(insert)
+ .withBufferSizeInMiB(1)
+ .build();
+ for (int i = 0; i < 100; i++)
+ {
+ LinkedHashMap<String, String> map = new LinkedHashMap<>();
+ map.put("a_key", "av" + i);
+ map.put("b_key", "zv" + i);
+ writer.addRow(String.valueOf(i), map);
+ }
+ for (int i = 100; i < 200; i++)
+ {
+ LinkedHashMap<String, String> map = new LinkedHashMap<>();
+ map.put("b_key", "zv" + i);
+ map.put("a_key", "av" + i);
+ writer.addRow(String.valueOf(i), map);
+ }
+ writer.close();
+ loadSSTables(dataDir, keyspace);
+
+ UntypedResultSet rs = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + ";");
+ assertEquals(200, rs.size());
+ Map<String, Map<String, String>> map = StreamSupport.stream(rs.spliterator(), false)
+ .collect(Collectors.toMap(r -> r.getString("k"), r -> r.getFrozenMap("c", UTF8Type.instance, UTF8Type.instance)));
+ for (int i = 0; i < 200; i++)
+ {
+ final String expectedKey = String.valueOf(i);
+ assertTrue(map.containsKey(expectedKey));
+ Map<String, String> innerMap = map.get(expectedKey);
+ assertTrue(innerMap.containsKey("a_key"));
+ assertEquals(innerMap.get("a_key"), "av" + i);
+ assertTrue(innerMap.containsKey("b_key"));
+ assertEquals(innerMap.get("b_key"), "zv" + i);
+ }
+
+ // Make sure we can filter with map values regardless of which order we put the keys in
+ UntypedResultSet filtered;
+ filtered = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + " where k='0' and c={'a_key': 'av0', 'b_key': 'zv0'};");
+ assertEquals(1, filtered.size());
+ filtered = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + " where k='0' and c={'b_key': 'zv0', 'a_key': 'av0'};");
+ assertEquals(1, filtered.size());
+ filtered = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + " where k='100' and c={'b_key': 'zv100', 'a_key': 'av100'};");
+ assertEquals(1, filtered.size());
+ filtered = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + " where k='100' and c={'a_key': 'av100', 'b_key': 'zv100'};");
+ assertEquals(1, filtered.size());
+ }
+
+ @Test
+ public void testFrozenMapTypeCustomOrdered() throws Exception
+ {
+ // Test to make sure we can write to `date` fields in both old and new formats
+ String schema = "CREATE TABLE " + qualifiedTable + " ("
+ + " k text,"
+ + " c frozen<map<timeuuid, int>>,"
+ + " PRIMARY KEY (k, c)"
+ + ")";
+ String insert = "INSERT INTO " + qualifiedTable + " (k, c) VALUES (?, ?)";
+ CQLSSTableWriter writer = CQLSSTableWriter.builder()
+ .inDirectory(dataDir)
+ .forTable(schema)
+ .using(insert)
+ .withBufferSizeInMiB(1)
+ .build();
+ UUID uuid1 = UUIDs.timeBased();
+ UUID uuid2 = UUIDs.timeBased();
+ UUID uuid3 = UUIDs.timeBased();
+ UUID uuid4 = UUIDs.timeBased();
+ Map<UUID, Integer> map = new LinkedHashMap<>();
+ // NOTE: if these two `put` calls are switched, the test passes
+ map.put(uuid2, 2);
+ map.put(uuid1, 1);
+ writer.addRow(String.valueOf(1), map);
+
+ Map<UUID, Integer> map2 = new LinkedHashMap<>();
+ map2.put(uuid3, 1);
+ map2.put(uuid4, 2);
+ writer.addRow(String.valueOf(2), map2);
+
+ writer.close();
+ loadSSTables(dataDir, keyspace);
+
+ UntypedResultSet rs = QueryProcessor.executeInternal("SELECT * FROM " + qualifiedTable + ";");
+ assertEquals(2, rs.size());
+ Map<String, Map<TimeUUID, Integer>> map3 = StreamSupport.stream(rs.spliterator(), false)
+ .collect(Collectors.toMap(r -> r.getString("k"),
+ r -> r.getFrozenMap("c",
+ TimeUUIDType.instance,
+ Int32Type.instance)));
Review Comment:
What's the purpose of this third map?
##########
test/unit/org/apache/cassandra/cql3/validation/operations/InsertTest.java:
##########
@@ -247,4 +252,42 @@ public void testCKInsertWithValueOver64K() throws Throwable
assertInvalidThrow(InvalidRequestException.class,
"INSERT INTO %s (a, b) VALUES ('foo', ?)", new String(TOO_BIG.array()));
}
+
+ @Test
+ public void testInsertingMapDataWithParameterizedQueriesIsKeyOrderIndependent() throws Throwable
+ {
+ UUID uuid1 = UUIDs.timeBased();
+ UUID uuid2 = UUIDs.timeBased();
+ createTable("CREATE TABLE %s (k text, c frozen<map<timeuuid, text>>, PRIMARY KEY (k, c));");
+ // We use `executeNet` in this test because `execute` passes parameters through CqlTester#transformValues(), which calls
+ // AbstractType#decompose() on the value, which "fixes" the map order, but wouldn't happen normally.
+ executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", buildInsertionOrderedMap(uuid1, "0", uuid2, "1"));
+ executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", buildInsertionOrderedMap(uuid2, "3", uuid1, "4"));
+ beforeAndAfterFlush(() -> {
+ assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid1 + ": '0', " + uuid2 + ": '1'}"), 1);
+ assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid2 + ": '1', " + uuid1 + ": '0'}"), 1);
+ assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid1 + ": '4', " + uuid2 + ": '3'}"), 1);
+ assertRowCountNet(executeNet("SELECT * FROM %s WHERE k='0' AND c={" + uuid2 + ": '3', " + uuid1 + ": '4'}"), 1);
+ });
+ }
+
+ @Test
+ public void testInsertingMapDataWithParameterizedQueriesIsKeyOrderIndependentWithSelectSlice() throws Throwable
+ {
+ UUID uuid1 = UUIDs.timeBased();
+ UUID uuid2 = UUIDs.timeBased();
+ createTable("CREATE TABLE %s (k text, c frozen<map<timeuuid, text>>, PRIMARY KEY (k, c));");
+ // We use `executeNet` in this test because `execute` passes parameters through CqlTester#transformValues(), which calls
+ // AbstractType#decompose() on the value, which "fixes" the map order, but wouldn't happen normally.
+ beforeAndAfterFlush(() -> {
+ executeNet("INSERT INTO %s (k, c) VALUES ('0', ?)", buildInsertionOrderedMap(uuid2, "2", uuid1, "1"));
+ // This works before the fix because uuid2 was inserted/serialized first
+ assertRowsNet(executeNet("SELECT k, c[" + uuid2 + "] from %s"), row("0", "2"));
+ // This fails because uuid1 is < uuid2, but is is serialized after,
+ // we short-circuit the projection once we think we're past the potential map key.
+ // Therefore, the query ends up returning `null` for the map value
Review Comment:
I would either update or remove the comments of the form "this works"/"this fails". These comments are useful in the context of presenting this bugfix but, once we had fixed the bug, the comments are obsolete because the bug is already fixed and the query doesn't fail anymore.
--
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