adelapena commented on code in PR #2556:
URL: https://github.com/apache/cassandra/pull/2556#discussion_r1289970266
##########
src/java/org/apache/cassandra/cql3/restrictions/ClusteringColumnRestrictions.java:
##########
@@ -92,61 +92,46 @@ public ClusteringColumnRestrictions mergeWith(Restriction
restriction, @Nullable
return new ClusteringColumnRestrictions(this.comparator,
newRestrictionSet, allowFiltering);
}
- private boolean hasMultiColumnSlice()
- {
- for (SingleRestriction restriction : restrictions)
- {
- if (restriction.isMultiColumn() && restriction.isSlice())
- return true;
- }
- return false;
- }
-
public NavigableSet<Clustering<?>> valuesAsClustering(QueryOptions
options, ClientState state) throws InvalidRequestException
{
- MultiCBuilder builder = MultiCBuilder.create(comparator, hasIN());
+ MultiCBuilder builder = MultiCBuilder.create(comparator);
for (SingleRestriction r : restrictions)
{
r.appendTo(builder, options);
if (hasIN() && Guardrails.inSelectCartesianProduct.enabled(state))
Review Comment:
I understand that the guardrails `partitionKeysInSelect` and
`inSelectCartesianProduct` shouldn't consider the new `NOT IN` restrictions.
That operator uses filtering and as such doesn't have the same performance
issues the guardrails try to protect us from.
##########
src/java/org/apache/cassandra/cql3/restrictions/ClusteringColumnRestrictions.java:
##########
@@ -92,61 +92,46 @@ public ClusteringColumnRestrictions mergeWith(Restriction
restriction, @Nullable
return new ClusteringColumnRestrictions(this.comparator,
newRestrictionSet, allowFiltering);
}
- private boolean hasMultiColumnSlice()
- {
- for (SingleRestriction restriction : restrictions)
- {
- if (restriction.isMultiColumn() && restriction.isSlice())
- return true;
- }
- return false;
- }
-
public NavigableSet<Clustering<?>> valuesAsClustering(QueryOptions
options, ClientState state) throws InvalidRequestException
{
- MultiCBuilder builder = MultiCBuilder.create(comparator, hasIN());
+ MultiCBuilder builder = MultiCBuilder.create(comparator);
for (SingleRestriction r : restrictions)
{
r.appendTo(builder, options);
if (hasIN() && Guardrails.inSelectCartesianProduct.enabled(state))
Guardrails.inSelectCartesianProduct.guard(builder.buildSize(),
"clustering key", false, state);
- if (builder.hasMissingElements())
+ if (builder.buildSize() == 0)
Review Comment:
There are four calls to `builder.buildSize() == 0`, maybe we could add an
`MultiCBuilder#isEmpty()` method?
##########
src/java/org/apache/cassandra/cql3/restrictions/SingleColumnRestriction.java:
##########
@@ -414,38 +398,56 @@ protected boolean isSupportedBy(Index index)
@Override
public String toString()
{
- return String.format("SLICE%s", slice);
+ return String.format("SLICE{%s, NOT IN %s}", slice, skippedValues);
}
- private SliceRestriction(ColumnMetadata columnDef, TermSlice slice)
- {
- super(columnDef);
- this.slice = slice;
- }
}
- // This holds CONTAINS, CONTAINS_KEY, and map[key] = value restrictions
because we might want to have any combination of them.
+
+ // This holds CONTAINS, CONTAINS_KEY, NOT CONTAINS, NOT CONTAINS KEY and
map[key] = value restrictions because we might want to have any combination of
them.
public static final class ContainsRestriction extends
SingleColumnRestriction
{
private final List<Term> values = new ArrayList<>(); // for CONTAINS
- private final List<Term> keys = new ArrayList<>(); // for CONTAINS_KEY
- private final List<Term> entryKeys = new ArrayList<>(); // for
map[key] = value
+ private final List<Term> negativeValues = new ArrayList<>(); // for
NOT_CONTAINS
Review Comment:
Nit: can we move the private constructor at the bottom of the class to the
top, so it's together with the other constructors? That way I think it would be
easier to understand that there can be instances with both positive and
negative term lists (from `doMergeWith`).
##########
src/java/org/apache/cassandra/cql3/restrictions/SingleColumnRestriction.java:
##########
@@ -414,38 +398,56 @@ protected boolean isSupportedBy(Index index)
@Override
public String toString()
{
- return String.format("SLICE%s", slice);
+ return String.format("SLICE{%s, NOT IN %s}", slice, skippedValues);
}
- private SliceRestriction(ColumnMetadata columnDef, TermSlice slice)
- {
- super(columnDef);
- this.slice = slice;
- }
}
- // This holds CONTAINS, CONTAINS_KEY, and map[key] = value restrictions
because we might want to have any combination of them.
+
+ // This holds CONTAINS, CONTAINS_KEY, NOT CONTAINS, NOT CONTAINS KEY and
map[key] = value restrictions because we might want to have any combination of
them.
public static final class ContainsRestriction extends
SingleColumnRestriction
{
private final List<Term> values = new ArrayList<>(); // for CONTAINS
- private final List<Term> keys = new ArrayList<>(); // for CONTAINS_KEY
- private final List<Term> entryKeys = new ArrayList<>(); // for
map[key] = value
+ private final List<Term> negativeValues = new ArrayList<>(); // for
NOT_CONTAINS
+ private List<Term> keys = new ArrayList<>(); // for CONTAINS_KEY
+ private final List<Term> negativeKeys = new ArrayList<>(); // for
NOT_CONTAINS_KEY
+ private List<Term> entryKeys = new ArrayList<>(); // for map[key] =
value
private final List<Term> entryValues = new ArrayList<>(); // for
map[key] = value
+ private List<Term> negativeEntryKeys = new ArrayList<>(); // for
map[key] != value
+ private List<Term> negativeEntryValues = new ArrayList<>(); // for
map[key] != value
Review Comment:
All can be `final`.
##########
src/java/org/apache/cassandra/db/MultiCBuilder.java:
##########
@@ -19,104 +19,155 @@
import java.nio.ByteBuffer;
import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.NavigableSet;
+import org.apache.cassandra.cql3.statements.Bound;
+import org.apache.cassandra.db.marshal.AbstractType;
import org.apache.cassandra.schema.ColumnMetadata;
import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.UniqueComparator;
import org.apache.cassandra.utils.btree.BTreeSet;
/**
* Builder that allow to build multiple Clustering/ClusteringBound at the same
time.
*/
-public abstract class MultiCBuilder
+public class MultiCBuilder
{
+ /**
+ * Represents a building block of a clustering.
+ * Either a point (single value) or a bound.
+ * Knowing the kind of elements greatly simplifies the process of building
clustering bounds.
+ */
+ public static class Element
Review Comment:
Nice utility class! It's not related to the patch but, since we are here, I
wonder if we could/should rename the class to something more explicit, like
`MultiClusteringBuilder`.
--
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]
