adelapena commented on code in PR #2916:
URL: https://github.com/apache/cassandra/pull/2916#discussion_r1400841316
##########
src/java/org/apache/cassandra/index/sai/plan/Expression.java:
##########
@@ -26,16 +26,31 @@
import org.slf4j.LoggerFactory;
import org.apache.cassandra.cql3.Operator;
-import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.index.sai.IndexContext;
+import org.apache.cassandra.index.sai.StorageAttachedIndex;
import org.apache.cassandra.index.sai.analyzer.AbstractAnalyzer;
-import org.apache.cassandra.index.sai.utils.TypeUtil;
+import org.apache.cassandra.index.sai.analyzer.NoOpAnalyzer;
+import org.apache.cassandra.index.sai.utils.IndexTermType;
-public class Expression
+public interface Expression
Review Comment:
The interface could use some JavaDoc explaining what an `Expression` is, and
that we have two subclasses for indexed/not-indexed columns.
##########
src/java/org/apache/cassandra/index/sai/memory/TrieMemoryIndex.java:
##########
@@ -65,22 +65,20 @@ public class TrieMemoryIndex extends MemoryIndex
private final InMemoryTrie<PrimaryKeys> data;
private final PrimaryKeysReducer primaryKeysReducer;
- private final AbstractAnalyzer.AnalyzerFactory analyzerFactory;
- private final AbstractType<?> validator;
+ private final IndexTermType indexTermType;
Review Comment:
We already have a reference to `StorageAttachedIndex` in the parent class,
and not that many uses of its contained `IndexTermType`, so maybe we can get
rid of this attribute and use `index.termType()` instead?
##########
src/java/org/apache/cassandra/index/sai/utils/IndexIdentifier.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.index.sai.utils;
+
+/**
+ * This is a simple wrapper around the index identity. It's primary purpose is
to isolate classes that only need
Review Comment:
```suggestion
* This is a simple wrapper around the index identity. Its primary purpose
is to isolate classes that only need
```
##########
src/java/org/apache/cassandra/index/sai/plan/Expression.java:
##########
@@ -106,244 +110,344 @@ public Expression(IndexContext indexContext)
* @param value the expression value
* @return the current expression with the added operation
*/
- public Expression add(Operator op, ByteBuffer value)
+ Expression add(Operator op, ByteBuffer value);
+
+ /**
+ * Used in post-filtering to determine is an indexed value matches the
expression
+ */
+ boolean isSatisfiedBy(ByteBuffer columnValue);
+
+ Bound lower();
+
+ Bound upper();
+
+ class IndexedExpression extends AbstractExpression
{
- boolean lowerInclusive, upperInclusive;
- // If the type supports rounding then we need to make sure that index
- // range search is always inclusive, otherwise we run the risk of
- // missing values that are within the exclusive range but are rejected
- // because their rounded value is the same as the value being queried.
- lowerInclusive = upperInclusive = TypeUtil.supportsRounding(validator);
- switch (op)
- {
- case EQ:
- case CONTAINS:
- case CONTAINS_KEY:
- lower = new Bound(value, validator, true);
- upper = lower;
- operator = IndexOperator.valueOf(op);
- break;
-
- case LTE:
- if (context.getDefinition().isReversedType())
- {
- this.lowerInclusive = true;
- lowerInclusive = true;
- }
- else
- {
- this.upperInclusive = true;
- upperInclusive = true;
- }
- case LT:
- operator = IndexOperator.RANGE;
- if (context.getDefinition().isReversedType())
- lower = new Bound(value, validator, lowerInclusive);
- else
- upper = new Bound(value, validator, upperInclusive);
- break;
+ private final StorageAttachedIndex index;
- case GTE:
- if (context.getDefinition().isReversedType())
- {
- this.upperInclusive = true;
- upperInclusive = true;
- }
- else
- {
- this.lowerInclusive = true;
- lowerInclusive = true;
- }
- case GT:
- operator = IndexOperator.RANGE;
- if (context.getDefinition().isReversedType())
- upper = new Bound(value, validator, upperInclusive);
- else
- lower = new Bound(value, validator, lowerInclusive);
- break;
- case ANN:
- operator = IndexOperator.ANN;
- lower = new Bound(value, validator, true);
- upper = lower;
- break;
+ public IndexedExpression(StorageAttachedIndex index)
+ {
+ super(index.termType());
+ this.index = index;
+ }
+
+ @Override
+ public boolean isNotIndexed()
+ {
+ return false;
}
- assert operator != null;
+ @Override
+ public StorageAttachedIndex getIndex()
+ {
+ return index;
+ }
- return this;
+ @Override
+ public AbstractAnalyzer getAnalyzer()
+ {
+ return index.analyzer();
+ }
}
- /**
- * Used in post-filtering to determine is an indexed value matches the
expression
- */
- public boolean isSatisfiedBy(ByteBuffer columnValue)
+ class UnindexedExpression extends AbstractExpression
{
- // If the expression represents an ANN ordering then we return true
because the actual result
- // is approximate and will rarely / never match the expression value
- if (validator.isVector())
+ private UnindexedExpression(IndexTermType indexTermType)
+ {
+ super(indexTermType);
+ }
+
+ @Override
+ public boolean isNotIndexed()
+ {
return true;
+ }
- if (!TypeUtil.isValid(columnValue, validator))
+ @Override
+ public StorageAttachedIndex getIndex()
{
- logger.error(context.logMessage("Value is not valid for indexed
column {} with {}"), context.getColumnName(), validator);
- return false;
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public AbstractAnalyzer getAnalyzer()
+ {
+ return new NoOpAnalyzer();
}
+ }
+
+ abstract class AbstractExpression implements Expression
Review Comment:
I'm not sure there is a benefit in having a separate interface and abstract
class for this.
##########
src/java/org/apache/cassandra/index/sai/utils/IndexIdentifier.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.index.sai.utils;
+
+/**
+ * This is a simple wrapper around the index identity. It's primary purpose is
to isolate classes that only need
+ * access to the identity from the main index classes. This is useful in
testing but also makes it easier to pass
+ * the log message wrapper {@link #logMessage(String)} to class that don't
need any other information about the index.
Review Comment:
```suggestion
* the log message wrapper {@link #logMessage(String)} to classes that don't
need any other information about the index.
```
--
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]
