adelapena commented on code in PR #2673: URL: https://github.com/apache/cassandra/pull/2673#discussion_r1327119333
########## src/java/org/apache/cassandra/cql3/Ordering.java: ########## @@ -0,0 +1,187 @@ +/* + * 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 org.apache.cassandra.cql3.restrictions.SingleColumnRestriction; +import org.apache.cassandra.cql3.restrictions.SingleRestriction; +import org.apache.cassandra.schema.ColumnMetadata; +import org.apache.cassandra.schema.TableMetadata; + +/** + * A single element of an ORDER BY clause. + * <code>ORDER BY ordering1 [, ordering2 [, ...]] </code> + * <p> + * An ordering comprises an expression that produces the values to compare against each other + * and a sorting direction (ASC, DESC). + */ +public class Ordering +{ + public final Expression expression; + public final Direction direction; + + public Ordering(Expression expression, Direction direction) + { + this.expression = expression; + this.direction = direction; + } + + public interface Expression + { + default boolean hasNonClusteredOrdering() + { + return false; + } + + default SingleRestriction toRestriction() + { + throw new UnsupportedOperationException(); + } + + ColumnMetadata getColumn(); Review Comment: All ordering expressions take a column of the constructor and have an identical implementation of `getColumn`. Maybe `Ordering.Expression` could be an abstract class instead of an interface? As in: ```java public static abstract class Expression { protected final ColumnMetadata column; public Expression(ColumnMetadata column) { this.column = column; } public boolean hasNonClusteredOrdering() { return false; } public SingleRestriction toRestriction() { throw new UnsupportedOperationException(); } public ColumnMetadata getColumn() { return column; } } ``` ########## src/java/org/apache/cassandra/cql3/SingleColumnRelation.java: ########## @@ -264,6 +265,16 @@ protected Restriction newLikeRestriction(TableMetadata table, VariableSpecificat return new SingleColumnRestriction.LikeRestriction(columnDef, operator, term); } + @Override + protected Restriction newAnnRestriction(TableMetadata table, VariableSpecifications boundNames) + { + ColumnMetadata columnDef = table.getExistingColumn(entity); + if (!(columnDef.type instanceof VectorType)) + throw invalidRequest("ANN is only supported against DENSE FLOAT32 columns"); Review Comment: The error message should probably mention the CQL vector type, instead of `DENSE FLOAT`. Also, so we have unit tests for this? ########## src/java/org/apache/cassandra/cql3/SingleColumnRelation.java: ########## @@ -264,6 +265,16 @@ protected Restriction newLikeRestriction(TableMetadata table, VariableSpecificat return new SingleColumnRestriction.LikeRestriction(columnDef, operator, term); } + @Override + protected Restriction newAnnRestriction(TableMetadata table, VariableSpecifications boundNames) + { + ColumnMetadata columnDef = table.getExistingColumn(entity); + if (!(columnDef.type instanceof VectorType)) Review Comment: Shouldn't we also check that the vector type is for floats? ########## src/java/org/apache/cassandra/cql3/statements/SelectStatement.java: ########## @@ -1573,6 +1656,39 @@ public int compare(List<ByteBuffer> a, List<ByteBuffer> b) } } + private static class IndexColumnComparator<T> extends ColumnComparator<List<ByteBuffer>> + { + private final SingleRestriction restriction; + private final int columnIndex; + + // VSTODO maybe cache in prepared statement + public IndexColumnComparator(SingleRestriction restriction, int columnIndex) + { + this.restriction = restriction; + this.columnIndex = columnIndex; + } + + @Override + public boolean indexOrdering() + { + return true; + } + + @Override + public Comparator<List<ByteBuffer>> prepareFor(TableMetadata table, QueryOptions options) + { + Index index = restriction.findSupportingIndex(IndexRegistry.obtain(table)); Review Comment: This finds the first of those indexes that support the restriction. However, the index selection for the query is made by `SIM#getBestIndexQueryPlanFor`. So I think a query using an index implementation could end up using the comparator provided by another index implementation. This doesn't happen in practice because SAI happens to be the only provided implementation using sorting, albeit it could still happen with custom implementations. ########## src/java/org/apache/cassandra/cql3/TokenRelation.java: ########## @@ -118,6 +118,12 @@ protected Restriction newLikeRestriction(TableMetadata table, VariableSpecificat throw invalidRequest("%s cannot be used with the token function", operator); } + @Override + protected Restriction newAnnRestriction(TableMetadata table, VariableSpecifications boundNames) + { + throw invalidRequest("%s cannot be used for toekn relations", operator()); Review Comment: Typo:s/toekn/token Also, maybe we should keep the style of the error messages above and use `with the token function` instead of `for token relations`. ########## src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java: ########## @@ -266,6 +278,8 @@ else if (relation.isLIKE()) // there is restrictions not covered by the PK. if (!nonPrimaryKeyRestrictions.isEmpty()) { + var columnRestrictions = allColumnRestrictions(clusteringColumnsRestrictions, nonPrimaryKeyRestrictions); Review Comment: This seems unused -- 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]
