Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce operation attributes #2333

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Introduce operation attributes #2333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ea2f57
Introduce operation attributes
brfrn169 Nov 7, 2024
241b26e
Fix based on feedback
brfrn169 Nov 14, 2024
c00f0b4
Fix based on feedback
brfrn169 Nov 14, 2024
106b19e
Merge branch 'master' into introduce-operation-attributes
brfrn169 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion core/src/main/java/com/scalar/db/api/Delete.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableMap;
import com.scalar.db.api.DeleteBuilder.BuildableFromExisting;
import com.scalar.db.api.DeleteBuilder.Namespace;
import com.scalar.db.io.Key;
Expand All @@ -25,8 +26,9 @@ public class Delete extends Mutation {
Key partitionKey,
@Nullable Key clusteringKey,
@Nullable Consistency consistency,
ImmutableMap<String, String> attributes,
@Nullable MutationCondition condition) {
super(namespace, tableName, partitionKey, clusteringKey, consistency, condition);
super(namespace, tableName, partitionKey, clusteringKey, consistency, attributes, condition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if there is any reason for putting it in the middle of existing arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that the first six arguments are for Operation. I arranged the arguments in hierarchical order: Operation, Mutation, and Delete.

}

/**
Expand Down Expand Up @@ -172,6 +174,7 @@ public String toString() {
.add("partitionKey", getPartitionKey())
.add("clusteringKey", getClusteringKey())
.add("consistency", getConsistency())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a super minor thing, but I think it's slightly better that the order is consistent with the order of the constructor arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me make them consistent. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c00f0b4. Thanks.

.add("attributes", getAttributes())
.add("condition", getCondition())
.toString();
}
Expand Down
63 changes: 59 additions & 4 deletions core/src/main/java/com/scalar/db/api/DeleteBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.collect.ImmutableMap;
import com.scalar.db.api.OperationBuilder.Attribute;
import com.scalar.db.api.OperationBuilder.ClearAttribute;
import com.scalar.db.api.OperationBuilder.ClearClusteringKey;
import com.scalar.db.api.OperationBuilder.ClearCondition;
import com.scalar.db.api.OperationBuilder.ClearNamespace;
Expand All @@ -11,6 +14,8 @@
import com.scalar.db.api.OperationBuilder.PartitionKeyBuilder;
import com.scalar.db.api.OperationBuilder.TableBuilder;
import com.scalar.db.io.Key;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;

public class DeleteBuilder {
Expand Down Expand Up @@ -60,10 +65,14 @@ public Buildable partitionKey(Key partitionKey) {
}

public static class Buildable extends OperationBuilder.Buildable<Delete>
implements ClusteringKey<Buildable>, Consistency<Buildable>, Condition<Buildable> {
implements ClusteringKey<Buildable>,
Consistency<Buildable>,
Condition<Buildable>,
Attribute<Buildable> {
@Nullable Key clusteringKey;
@Nullable com.scalar.db.api.Consistency consistency;
@Nullable MutationCondition condition;
final Map<String, String> attributes = new HashMap<>();

private Buildable(@Nullable String namespace, String table, Key partitionKey) {
super(namespace, table, partitionKey);
Expand All @@ -90,10 +99,31 @@ public Buildable consistency(com.scalar.db.api.Consistency consistency) {
return this;
}

@Override
public Buildable attribute(String name, String value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's again a super minor comment, but the definition order of the variables (consistency, condition, and attributes) and the method definition order can be consistent to make it slightly better.
It's most probably my preference, so I don't argue about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me make them consistent. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought I would do that this time, but it looks like it’s very time-consuming… Actually, the original method definition order is already inconsistent. Let’s tackle it when we have more time. 🙇

checkNotNull(name);
checkNotNull(value);
attributes.put(name, value);
return this;
}

@Override
public Buildable attributes(Map<String, String> attributes) {
checkNotNull(attributes);
this.attributes.putAll(attributes);
return this;
}

@Override
public Delete build() {
return new Delete(
namespaceName, tableName, partitionKey, clusteringKey, consistency, condition);
namespaceName,
tableName,
partitionKey,
clusteringKey,
consistency,
ImmutableMap.copyOf(attributes),
condition);
}
}

Expand All @@ -103,17 +133,18 @@ public static class BuildableFromExisting extends Buildable
OperationBuilder.PartitionKey<BuildableFromExisting>,
ClearCondition<BuildableFromExisting>,
ClearClusteringKey<BuildableFromExisting>,
ClearNamespace<BuildableFromExisting> {
ClearNamespace<BuildableFromExisting>,
ClearAttribute<BuildableFromExisting> {

BuildableFromExisting(Delete delete) {
super(
delete.forNamespace().orElse(null),
delete.forTable().orElse(null),
delete.getPartitionKey());

this.clusteringKey = delete.getClusteringKey().orElse(null);
this.consistency = delete.getConsistency();
this.condition = delete.getCondition().orElse(null);
this.attributes.putAll(delete.getAttributes());
}

@Override
Expand Down Expand Up @@ -149,6 +180,18 @@ public BuildableFromExisting consistency(com.scalar.db.api.Consistency consisten
return this;
}

@Override
public BuildableFromExisting attribute(String name, String value) {
super.attribute(name, value);
return this;
}

@Override
public BuildableFromExisting attributes(Map<String, String> attributes) {
super.attributes(attributes);
return this;
}

@Override
public BuildableFromExisting condition(MutationCondition condition) {
super.condition(condition);
Expand All @@ -172,5 +215,17 @@ public BuildableFromExisting clearNamespace() {
this.namespaceName = null;
return this;
}

@Override
public BuildableFromExisting clearAttributes() {
this.attributes.clear();
return this;
}

@Override
public BuildableFromExisting clearAttribute(String name) {
this.attributes.remove(name);
return this;
}
}
}
14 changes: 12 additions & 2 deletions core/src/main/java/com/scalar/db/api/Get.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.scalar.db.api.GetBuilder.BuildableGetOrGetWithIndexFromExisting;
import com.scalar.db.api.GetBuilder.Namespace;
Expand All @@ -26,10 +27,18 @@ public class Get extends Selection {
Key partitionKey,
@Nullable Key clusteringKey,
@Nullable Consistency consistency,
ImmutableMap<String, String> attributes,
List<String> projections,
ImmutableSet<Conjunction> conjunctions) {
super(
namespace, tableName, partitionKey, clusteringKey, consistency, projections, conjunctions);
namespace,
tableName,
partitionKey,
clusteringKey,
consistency,
attributes,
projections,
conjunctions);
}

/**
Expand Down Expand Up @@ -178,9 +187,10 @@ public String toString() {
.add("table", forTable())
.add("partitionKey", getPartitionKey())
.add("clusteringKey", getClusteringKey())
.add("consistency", getConsistency())
.add("attributes", getAttributes())
.add("projections", getProjections())
.add("conjunctions", getConjunctions())
.add("consistency", getConsistency())
.toString();
}
}
Loading