adelapena commented on code in PR #2673:
URL: https://github.com/apache/cassandra/pull/2673#discussion_r1331686334


##########
src/java/org/apache/cassandra/cql3/statements/SelectStatement.java:
##########
@@ -483,7 +486,7 @@ else if (restrictions.keyIsInRelation())
 
         // Please note that the isExhausted state of the pager only gets 
updated when we've closed the page, so this
         // shouldn't be moved inside the 'try' above.
-        if (!pager.isExhausted())
+        if (!pager.isExhausted() && !pager.pager.isTopK())

Review Comment:
   While testing how paging works, I have randomly found that a range filter on 
the partition key can throw an exception:
   ```java
   @Test
   public void testKeyRestrictions()
   {
       createTable("CREATE TABLE %s (k int PRIMARY KEY, v vector<float, 1>)");
       createIndex("CREATE CUSTOM INDEX ON %s(v) USING 'StorageAttachedIndex' 
WITH OPTIONS = {'similarity_function' : 'euclidean'}");
       execute("INSERT INTO %s (k, v) VALUES (1, [1])");
   
       assertRows(execute("SELECT k, v FROM %s WHERE k > 0 LIMIT 4 ALLOW 
FILTERING"), row(1, vector(1f)));
       assertRows(execute("SELECT k, v FROM %s WHERE k = 1 ORDER BY v ANN OF 
[0] LIMIT 4 ALLOW FILTERING"), row(1, vector(1f)));
       assertRows(execute("SELECT k, v FROM %s WHERE k > 0 ORDER BY v ANN OF 
[0] LIMIT 4 ALLOW FILTERING"), row(1, vector(1f))); // exception!!
   }
   ```
   The error for in-memory is:
   ```
   java.lang.AssertionError: Attempt to use memtable index manager on 
non-indexed context
   
        at 
org.apache.cassandra.index.sai.IndexContext.getMemtableIndexManager(IndexContext.java:231)
        at 
org.apache.cassandra.index.sai.plan.vector.VectorQueryController.lambda$getIndexQueryResults$1(VectorQueryController.java:89)
        at 
java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271)
        at 
java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
        at 
java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
        at 
java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at 
java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at 
java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at 
java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at 
org.apache.cassandra.index.sai.plan.vector.VectorQueryController.getIndexQueryResults(VectorQueryController.java:90)
        at 
org.apache.cassandra.index.sai.plan.Operation$AndNode.rangeIterator(Operation.java:305)
        at 
org.apache.cassandra.index.sai.plan.Operation.buildIterator(Operation.java:178)
        at 
org.apache.cassandra.index.sai.plan.StorageAttachedIndexSearcher$ResultRetriever.<init>(StorageAttachedIndexSearcher.java:152)
        at 
org.apache.cassandra.index.sai.plan.StorageAttachedIndexSearcher.lambda$search$0(StorageAttachedIndexSearcher.java:111)
        at 
org.apache.cassandra.index.sai.plan.StorageAttachedIndexSearcher.search(StorageAttachedIndexSearcher.java:118)
        at 
org.apache.cassandra.db.ReadCommand.executeLocally(ReadCommand.java:431)
        at 
org.apache.cassandra.db.AbstractReadQuery.executeInternal(AbstractReadQuery.java:65)
        at 
org.apache.cassandra.db.ReadCommand.executeInternal(ReadCommand.java:88)
        at 
org.apache.cassandra.cql3.statements.SelectStatement.executeInternal(SelectStatement.java:543)
        at 
org.apache.cassandra.cql3.statements.SelectStatement.executeLocally(SelectStatement.java:515)
        at 
org.apache.cassandra.cql3.statements.SelectStatement.executeLocally(SelectStatement.java:108)
        at 
org.apache.cassandra.cql3.QueryProcessor.executeInternal(QueryProcessor.java:445)
        at 
org.apache.cassandra.cql3.CQLTester.executeFormattedQuery(CQLTester.java:1595)
        at org.apache.cassandra.cql3.CQLTester.execute(CQLTester.java:1574)
        at 
org.apache.cassandra.cql3.validation.operations.CQLVectorTest.testKeyRestrictions(CQLVectorTest.java:69)
        at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
        at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
        at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
        at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
        at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
        at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
        at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
        at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
        at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
        at 
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
        at 
com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
        at 
com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
        at 
com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
        at 
com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:232)
        at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:55)
   ```
   Whereas if we flush the table it looks like this:
   ```
   java.lang.UnsupportedOperationException
        at 
org.apache.cassandra.io.filesystem.ListenableFileSystem$ListenablePath.toFile(ListenableFileSystem.java:661)
        at org.apache.cassandra.io.util.File.toJavaIOFile(File.java:724)
        at 
org.apache.cassandra.index.sai.disk.format.IndexDescriptor.createComponentOnDisk(IndexDescriptor.java:185)
        at 
org.apache.cassandra.index.sai.disk.v1.SSTableComponentsWriter.complete(SSTableComponentsWriter.java:106)
        at 
org.apache.cassandra.index.sai.disk.StorageAttachedIndexWriter.complete(StorageAttachedIndexWriter.java:171)
        at 
com.google.common.collect.ImmutableList.forEach(ImmutableList.java:422)
        at 
org.apache.cassandra.io.sstable.format.SSTableWriter.commit(SSTableWriter.java:235)
        at 
org.apache.cassandra.io.sstable.SimpleSSTableMultiWriter.commit(SimpleSSTableMultiWriter.java:90)
        at 
org.apache.cassandra.db.ColumnFamilyStore$Flush.flushMemtable(ColumnFamilyStore.java:1321)
        at 
org.apache.cassandra.db.ColumnFamilyStore$Flush.run(ColumnFamilyStore.java:1219)
        at 
org.apache.cassandra.concurrent.ExecutionFailure$1.run(ExecutionFailure.java:133)
        at 
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at 
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at 
io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:834)
   ```



##########
src/java/org/apache/cassandra/cql3/statements/SelectStatement.java:
##########
@@ -288,7 +291,7 @@ public ResultMessage.Rows execute(QueryState state, 
QueryOptions options, long q
             query.trackWarnings();
         ResultMessage.Rows rows;
 
-        if (aggregationSpec == null && (pageSize <= 0 || 
(query.limits().count() <= pageSize)))
+        if (aggregationSpec == null && (pageSize <= 0 || 
(query.limits().count() <= pageSize) || query.isTopK()))

Review Comment:
   Also, maybe we should throw a client warning if the query's page size is 
lower than the limit.



-- 
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]

Reply via email to