Fix JSON queries with IN restrictions and ORDER BY clause patch by Francisco Fernandez; reviewed by Benjamin Lerer for CASSANDRA-14286
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/594cde79 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/594cde79 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/594cde79 Branch: refs/heads/cassandra-3.11 Commit: 594cde7937de6f848998bac35d35591f8a18aa10 Parents: b3ac793 Author: Francisco Fernandez <francisco.fernandez.cast...@gmail.com> Authored: Tue Apr 17 12:02:25 2018 +0200 Committer: Benjamin Lerer <b.le...@gmail.com> Committed: Tue Apr 17 12:04:33 2018 +0200 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../org/apache/cassandra/cql3/ResultSet.java | 5 ++ .../cassandra/cql3/selection/Selection.java | 69 +++++++++++++++++--- .../cql3/statements/SelectStatement.java | 19 +++--- .../cql3/validation/entities/JsonTest.java | 21 ++++++ 5 files changed, 96 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/594cde79/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 5221b1e..967ee05 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 2.2.13 + * Fix JSON queries with IN restrictions and ORDER BY clause (CASSANDRA-14286) * CQL fromJson(null) throws NullPointerException (CASSANDRA-13891) * Fix query pager DEBUG log leak causing hit in paged reads throughput (CASSANDRA-14318) * Backport circleci yaml (CASSANDRA-14240) http://git-wip-us.apache.org/repos/asf/cassandra/blob/594cde79/src/java/org/apache/cassandra/cql3/ResultSet.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/ResultSet.java b/src/java/org/apache/cassandra/cql3/ResultSet.java index 028691f..16f0d1b 100644 --- a/src/java/org/apache/cassandra/cql3/ResultSet.java +++ b/src/java/org/apache/cassandra/cql3/ResultSet.java @@ -254,6 +254,11 @@ public class ResultSet return new ResultMetadata(EnumSet.copyOf(flags), names, columnCount, pagingState); } + public int getColumnCount() + { + return columnCount; + } + /** * Return only the column names requested by the user, excluding those added for post-query re-orderings, * see definition of names and columnCount. http://git-wip-us.apache.org/repos/asf/cassandra/blob/594cde79/src/java/org/apache/cassandra/cql3/selection/Selection.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/selection/Selection.java b/src/java/org/apache/cassandra/cql3/selection/Selection.java index dc4e240..5385fc6 100644 --- a/src/java/org/apache/cassandra/cql3/selection/Selection.java +++ b/src/java/org/apache/cassandra/cql3/selection/Selection.java @@ -24,6 +24,7 @@ import com.google.common.base.Objects; import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; +import com.google.common.collect.Lists; import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.ColumnDefinition; @@ -56,6 +57,8 @@ public abstract class Selection private final ResultSet.ResultMetadata metadata; private final boolean collectTimestamps; private final boolean collectTTLs; + // Columns used to order the result set for multi-partition queries + private Map<ColumnDefinition, Integer> orderingIndex; protected Selection(CFMetaData cfm, List<ColumnDefinition> columns, @@ -130,6 +133,24 @@ public abstract class Selection return false; } + public Map<ColumnDefinition, Integer> getOrderingIndex(boolean isJson) + { + if (!isJson) + return orderingIndex; + + // If we order post-query in json, the first and only column that we ship to the client is the json column. + // In that case, we should keep ordering columns around to perform the ordering, then these columns will + // be placed after the json column. As a consequence of where the colums are placed, we should give the + // ordering index a value based on their position in the json encoding and discard the original index. + // (CASSANDRA-14286) + int columnIndex = 1; + Map<ColumnDefinition, Integer> jsonOrderingIndex = new LinkedHashMap<>(orderingIndex.size()); + for (ColumnDefinition column : orderingIndex.keySet()) + jsonOrderingIndex.put(column, columnIndex++); + + return jsonOrderingIndex; + } + public ResultSet.ResultMetadata getResultMetadata(boolean isJson) { if (!isJson) @@ -137,7 +158,13 @@ public abstract class Selection ColumnSpecification firstColumn = metadata.names.get(0); ColumnSpecification jsonSpec = new ColumnSpecification(firstColumn.ksName, firstColumn.cfName, Json.JSON_COLUMN_ID, UTF8Type.instance); - return new ResultSet.ResultMetadata(Arrays.asList(jsonSpec)); + ResultSet.ResultMetadata resultMetadata = new ResultSet.ResultMetadata(Lists.newArrayList(jsonSpec)); + if (orderingIndex != null) + { + for (ColumnDefinition orderingColumn : orderingIndex.keySet()) + resultMetadata.addNonSerializedColumn(orderingColumn); + } + return resultMetadata; } public static Selection wildcard(CFMetaData cfm) @@ -152,7 +179,20 @@ public abstract class Selection return new SimpleSelection(cfm, columns, false); } - public int addColumnForOrdering(ColumnDefinition c) + public void addColumnForOrdering(ColumnDefinition c) + { + if (orderingIndex == null) + orderingIndex = new LinkedHashMap<>(); + + int index = getResultSetIndex(c); + + if (index < 0) + index = addOrderingColumn(c); + + orderingIndex.put(c, index); + } + + protected int addOrderingColumn(ColumnDefinition c) { columns.add(c); metadata.addNonSerializedColumn(c); @@ -356,14 +396,25 @@ public abstract class Selection private List<ByteBuffer> getOutputRow(int protocolVersion) { List<ByteBuffer> outputRow = selectors.getOutputRow(protocolVersion); - return isJson ? rowToJson(outputRow, protocolVersion) - : outputRow; + if (isJson) + { + List<ByteBuffer> jsonRow = rowToJson(outputRow, protocolVersion); + + // Keep ordering columns around for possible post-query ordering. (CASSANDRA-14286) + if (orderingIndex != null) + { + for (Integer orderingColumnIndex : orderingIndex.values()) + jsonRow.add(outputRow.get(orderingColumnIndex)); + } + outputRow = jsonRow; + } + return outputRow; } private List<ByteBuffer> rowToJson(List<ByteBuffer> row, int protocolVersion) { StringBuilder sb = new StringBuilder("{"); - for (int i = 0; i < metadata.names.size(); i++) + for (int i = 0; i < metadata.getColumnCount(); i++) { if (i > 0) sb.append(", "); @@ -383,7 +434,9 @@ public abstract class Selection sb.append(spec.type.toJSONString(buffer, protocolVersion)); } sb.append("}"); - return Collections.singletonList(UTF8Type.instance.getSerializer().serialize(sb.toString())); + List<ByteBuffer> jsonRow = new ArrayList<>(); + jsonRow.add(UTF8Type.instance.getSerializer().serialize(sb.toString())); + return jsonRow; } private ByteBuffer value(Cell c) @@ -515,9 +568,9 @@ public abstract class Selection } @Override - public int addColumnForOrdering(ColumnDefinition c) + protected int addOrderingColumn(ColumnDefinition c) { - int index = super.addColumnForOrdering(c); + int index = super.addOrderingColumn(c); factories.addSelectorForOrdering(c, index); return factories.size() - 1; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/594cde79/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index 13276c7..729cf83 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -842,7 +842,7 @@ public class SelectStatement implements CQLStatement if (!parameters.orderings.isEmpty()) { verifyOrderingIsAllowed(restrictions); - orderingComparator = getOrderingComparator(cfm, selection, restrictions); + orderingComparator = getOrderingComparator(cfm, selection, restrictions, parameters.isJson); isReversed = isReversed(cfm); } @@ -942,13 +942,14 @@ public class SelectStatement implements CQLStatement private Comparator<List<ByteBuffer>> getOrderingComparator(CFMetaData cfm, Selection selection, - StatementRestrictions restrictions) + StatementRestrictions restrictions, + boolean isJson) throws InvalidRequestException { if (!restrictions.keyIsInRelation()) return null; - Map<ColumnIdentifier, Integer> orderingIndexes = getOrderingIndex(cfm, selection); + Map<ColumnDefinition, Integer> orderingIndexes = getOrderingIndex(cfm, selection, isJson); List<Integer> idToSort = new ArrayList<Integer>(); List<Comparator<ByteBuffer>> sorters = new ArrayList<Comparator<ByteBuffer>>(); @@ -957,32 +958,28 @@ public class SelectStatement implements CQLStatement { ColumnIdentifier identifier = raw.prepare(cfm); ColumnDefinition orderingColumn = cfm.getColumnDefinition(identifier); - idToSort.add(orderingIndexes.get(orderingColumn.name)); + idToSort.add(orderingIndexes.get(orderingColumn)); sorters.add(orderingColumn.type); } return idToSort.size() == 1 ? new SingleColumnComparator(idToSort.get(0), sorters.get(0)) : new CompositeComparator(sorters, idToSort); } - private Map<ColumnIdentifier, Integer> getOrderingIndex(CFMetaData cfm, Selection selection) + private Map<ColumnDefinition, Integer> getOrderingIndex(CFMetaData cfm, Selection selection, boolean isJson) throws InvalidRequestException { // If we order post-query (see orderResults), the sorted column needs to be in the ResultSet for sorting, // even if we don't // ultimately ship them to the client (CASSANDRA-4911). - Map<ColumnIdentifier, Integer> orderingIndexes = new HashMap<>(); for (ColumnIdentifier.Raw raw : parameters.orderings.keySet()) { ColumnIdentifier column = raw.prepare(cfm); final ColumnDefinition def = cfm.getColumnDefinition(column); if (def == null) handleUnrecognizedOrderingColumn(column); - int index = selection.getResultSetIndex(def); - if (index < 0) - index = selection.addColumnForOrdering(def); - orderingIndexes.put(def.name, index); + selection.addColumnForOrdering(def); } - return orderingIndexes; + return selection.getOrderingIndex(isJson); } private boolean isReversed(CFMetaData cfm) throws InvalidRequestException http://git-wip-us.apache.org/repos/asf/cassandra/blob/594cde79/test/unit/org/apache/cassandra/cql3/validation/entities/JsonTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/JsonTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/JsonTest.java index 46cdd52..9d40038 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/JsonTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/JsonTest.java @@ -1264,4 +1264,25 @@ public class JsonTest extends CQLTester executor.shutdown(); Assert.assertTrue(executor.awaitTermination(30, TimeUnit.SECONDS)); } + + // CASSANDRA-14286 + @Test + public void testJsonOrdering() throws Throwable + { + createTable("CREATE TABLE %s( PRIMARY KEY (a, b), a INT, b INT);"); + execute("INSERT INTO %s(a, b) VALUES (20, 30);"); + execute("INSERT INTO %s(a, b) VALUES (100, 200);"); + + assertRows(execute("SELECT JSON a, b FROM %s WHERE a IN (20, 100) ORDER BY b"), + row("{\"a\": 20, \"b\": 30}"), + row("{\"a\": 100, \"b\": 200}")); + + assertRows(execute("SELECT JSON a, b FROM %s WHERE a IN (20, 100) ORDER BY b DESC"), + row("{\"a\": 100, \"b\": 200}"), + row("{\"a\": 20, \"b\": 30}")); + + assertRows(execute("SELECT JSON a FROM %s WHERE a IN (20, 100) ORDER BY b DESC"), + row("{\"a\": 100}"), + row("{\"a\": 20}")); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
