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.

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

[WIP] Rocks condition with update #22

Open
wants to merge 89 commits into
base: rocks-condition-expr-rebase
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
82faa37
rebase on main
stevelorddremio Feb 12, 2021
e241645
Rocks checkstyle
stevelorddremio Feb 13, 2021
451dc1d
Rocks PR updates. Correct AbstractTestStore put test for failing cond…
stevelorddremio Feb 14, 2021
bf5ba27
Rocks PR updates. Revert mongodb changes.
stevelorddremio Feb 15, 2021
2e2ca1c
RocksDB Ext PR updates
stevelorddremio Feb 17, 2021
6760853
RocksDB ext PR updates
stevelorddremio Feb 17, 2021
830a5b6
Rocks ext PR updates
stevelorddremio Feb 17, 2021
a85e2e3
Rocks update - initial skeleton
stevelorddremio Feb 13, 2021
eb77566
Rocks update add update visitor and start implementing update on L1
stevelorddremio Feb 13, 2021
3c59809
Rocks update
stevelorddremio Feb 13, 2021
e4da222
Rocks UpdateExpression SetClause test
stevelorddremio Feb 14, 2021
96d9ff4
Rocks Update L1 Id Set and Remove tests
stevelorddremio Feb 15, 2021
d66113c
Rocks update() consolidate tests
stevelorddremio Feb 15, 2021
a39be60
Rocks Update() consolidate tests
stevelorddremio Feb 15, 2021
70d94ed
RocksDB update() streams initial handling.
stevelorddremio Feb 16, 2021
b62cb30
RocksDB Revert Function back to be used purely for ConditionExpressio…
stevelorddremio Feb 16, 2021
6369e55
Rocks rebase
stevelorddremio Feb 17, 2021
93e10de
Rocks update for L1 children, ancestors and complete key list
stevelorddremio Feb 17, 2021
b2a50ea
Fixed up some compile issues.
Feb 23, 2021
991541f
Refactored and further implemented update for RocksL1.
Feb 23, 2021
fe1ca73
Refactored the update unit tests.
Feb 23, 2021
195cb03
Implemented update for RocksL2.
Feb 24, 2021
95c3b78
Implemented update for RocksL3.
Feb 24, 2021
2235baf
Partially mplemented update for RocksFragment.
Feb 24, 2021
f687c6c
Partially implemented update for RocksRef. Some code cleanup.
Feb 24, 2021
b50ee84
Implemented update for RocksValue and RocksWrappedValue.
Feb 25, 2021
35faf80
Implemented update for RocksCommitMetadata.
Feb 25, 2021
a0ec3f1
Reworked the logic for update. Now simpler operations are pushed down…
Feb 26, 2021
acfa4b5
Now the path is passed down to the update operations in the child cla…
Feb 26, 2021
7383a99
Fixed up checkstyle errors.
Feb 26, 2021
1897dd3
Rocks add missing AddClause visitor handling.
stevelorddremio Feb 27, 2021
9f03244
Implemented update for key mutations in RocksL1.
Mar 1, 2021
3da86a5
Fixed up issues when processing multiple update functions. Added unit…
Mar 2, 2021
a4c15a4
Implemented update for RocksDBStore. Implemented update operations fo…
Mar 3, 2021
14183e1
Updated based on PR feedback.
Mar 3, 2021
174cb9e
Corrected the wording of a comment.
Mar 4, 2021
f89c15b
Ensure that the minimum entities are created.
Mar 4, 2021
75c3552
Updated the field names to match what Dynamo is using.
Mar 4, 2021
97682b0
Fixed some compile errors.
Mar 5, 2021
4ee03d0
Changed the builder names back to what they used to be for the update…
Mar 5, 2021
b65deae
Rocks update add update visitor and start implementing update on L1
stevelorddremio Feb 13, 2021
b82bb9a
RocksDB Revert Function back to be used purely for ConditionExpressio…
stevelorddremio Feb 16, 2021
ef42414
Partially implemented update for RocksRef. Some code cleanup.
Feb 24, 2021
cba4685
Implemented update for RocksCommitMetadata.
Feb 25, 2021
0413574
Reworked the logic for update. Now simpler operations are pushed down…
Feb 26, 2021
ac180df
Now the path is passed down to the update operations in the child cla…
Feb 26, 2021
ade4e47
Fixed up checkstyle errors.
Feb 26, 2021
daa2224
Updated based on PR feedback.
Mar 3, 2021
8fde5f2
Updated the field names to match what Dynamo is using.
Mar 4, 2021
d7c5f61
Rocks add update handling for branch.commits id, commit and parent at…
stevelorddremio Mar 2, 2021
861038b
RocksDB Branch.commits.delta remove, set, appendToList handling added
stevelorddremio Mar 3, 2021
a46d15e
Rocks Branch Delta update, add tests and make them parameterized.
stevelorddremio Mar 3, 2021
4355b3c
RocksDB update() for branch commits delta
stevelorddremio Mar 4, 2021
bdafee5
Rocks update branch commits delta implementation - update tests
stevelorddremio Mar 4, 2021
1fe9354
Rocks branch.commits removal added.
stevelorddremio Mar 4, 2021
0bdbbfd
Checkstyle
stevelorddremio Mar 4, 2021
2859a1b
Rocks update complex types for branch.commits
stevelorddremio Mar 6, 2021
4dbce25
Rocks rebase on rocks-condition-with-update
stevelorddremio Mar 8, 2021
177a0c2
Added some comments to describe the structures that are stored.
Mar 8, 2021
9a0d9de
Updated the Javadoc comments for the Rocks entities. Added a new clas…
Mar 8, 2021
6a86fd6
Refactored some code to make it more readable for updates.
Mar 9, 2021
33d20e3
Updated to get the AbstractITTieredVersionStore tests running and pas…
Mar 9, 2021
dc8b996
Implemented more of remove for RocksRef. Added some Javadoc comments …
Mar 9, 2021
f6de458
Updated EntityConverter based on PR feedback.
Mar 9, 2021
38af3a5
Removed some checks that were reduntant.
Mar 9, 2021
d4adde7
Fixed a typo.
Mar 9, 2021
59df7bb
Updated PathPattern based on PR feedback.
Mar 9, 2021
1cdeb7c
Updated RocksBaseValue based on PR feedback.
Mar 10, 2021
1e698b7
Removed stream collection to a list where possible.
Mar 10, 2021
eab0e95
Rocks commits appendToList added
stevelorddremio Mar 9, 2021
d0b5d4e
Rocks add commits tests
stevelorddremio Mar 9, 2021
1fdaae4
Rocks add set equals for commits
stevelorddremio Mar 10, 2021
c9c27d2
Rocks update() for branch.commits.delta
stevelorddremio Mar 10, 2021
f9c4217
Rocks branch.commits.delta and key mutations appendToList added
stevelorddremio Mar 11, 2021
7289cc0
Refactored to make more extensive use of PathPattern. PathPattern now…
Mar 11, 2021
2123d6c
Updated the PathPattern to support prefix matching.
Mar 11, 2021
ccb9c42
Removed the fieldIsList() method.
Mar 11, 2021
fa385b6
Added final to a local variable.
Mar 11, 2021
2ec82e7
Updated some Javadoc comments.
Mar 11, 2021
a947cc2
Rocks add branch.commits.keys set-equals handling
stevelorddremio Mar 11, 2021
f0b903a
Rocks checkstyle
stevelorddremio Mar 11, 2021
b5d5e76
Merge branch 'rocks-condition-with-update' into rocks-update-branch
stevelorddremio Mar 11, 2021
409ff56
Rocks refactor branch.commits set equals into new structure.
stevelorddremio Mar 11, 2021
ff7e849
Moved the PathPattern creation to static variables.
Mar 11, 2021
4714642
Rocks branch.commits.keys.key appendToList impl
stevelorddremio Mar 12, 2021
f2b3b1e
Merge remote-tracking branch 'origin/rocks-condition-with-update' int…
stevelorddremio Mar 12, 2021
27d5731
Rocks checkstyle
stevelorddremio Mar 12, 2021
e2541b0
Rocks internal PR comments.
stevelorddremio Mar 12, 2021
25817ca
Rocks Enable disabled tests
stevelorddremio Mar 13, 2021
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/*
* Copyright (C) 2020 Dremio
*
* Licensed 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.projectnessie.versioned.rocksdb;

import java.util.Map;
import java.util.stream.Collectors;

import org.projectnessie.versioned.store.Entity;

public class EntityConverter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why public? JavaDocs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No reason for public. Added JavaDocs.


/**
* This converts an Entity into Commit protobuf object.
* @param entity the entity to convert
* @return the object converted from the entity
*/
public static ValueProtos.Commit entityToCommit(Entity entity) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why public?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed to package.

if (!entity.getType().equals(Entity.EntityType.MAP)) {
throw new UnsupportedOperationException();
}
ValueProtos.Commit.Builder builder = ValueProtos.Commit.newBuilder();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style should be that we append final to everything, unless you have a reason to do so otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated.


for (Map.Entry<String, Entity> level0 : entity.getMap().entrySet()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this throw already if it's not a map?

switch (level0.getKey()) {
case RocksRef.COMMITS_ID:
builder.setId(level0.getValue().getBinary());
break;
case RocksRef.COMMITS_PARENT:
builder.setParent(level0.getValue().getBinary());
break;
case RocksRef.COMMITS_COMMIT:
builder.setCommit(level0.getValue().getBinary());
break;
case RocksRef.COMMITS_DELTA:
if (!level0.getValue().getType().equals(Entity.EntityType.LIST)) {
throw new UnsupportedOperationException();
}
for (Entity level1 : level0.getValue().getList()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this throw if it's not a list already?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Etc. for all other checks of this sort.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct. Removed the checks.

builder.addDelta(entityToDelta(level1));
}
break;
case RocksRef.COMMITS_KEY_LIST:
if (!level0.getValue().getType().equals(Entity.EntityType.LIST)) {
throw new UnsupportedOperationException();
}
for (Entity level1 : level0.getValue().getList()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

level0.getValue().getList().stream().map(v -> entityToKeyMutuation(v)).forEach(k -> builder.addKeyMutation(k);
?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated.

builder.addKeyMutation(entityToKeyMutation(level1));
}
break;
default:
throw new UnsupportedOperationException();
}
}
return builder.build();
}

/**
* This converts an Entity into Delta protobuf object.
* @param entity the entity to convert
* @return the object converted from the entity
*/
public static ValueProtos.Delta entityToDelta(Entity entity) {
if (!entity.getType().equals(Entity.EntityType.MAP)) {
throw new UnsupportedOperationException();
}

ValueProtos.Delta.Builder builder = ValueProtos.Delta.newBuilder();

for (Map.Entry<String, Entity> level0 : entity.getMap().entrySet()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these all called level0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Renamed.

switch (level0.getKey()) {
case RocksRef.COMMITS_POSITION:
builder.setPosition((int) level0.getValue().getNumber());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use Ints.saturatedCast() if available.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated.

break;
case RocksRef.COMMITS_OLD_ID:
builder.setOldId(level0.getValue().getBinary());
break;
case RocksRef.COMMITS_NEW_ID:
builder.setNewId(level0.getValue().getBinary());
break;
default:
throw new UnsupportedOperationException();
}
}
return builder.build();
}

/**
* This converts an Entity into KeyMutation protobuf object.
* @param entity the entity to convert
* @return the object converted from the entity
*/
public static ValueProtos.KeyMutation entityToKeyMutation(Entity entity) {
if (!entity.getType().equals(Entity.EntityType.MAP)) {
throw new UnsupportedOperationException();
}

ValueProtos.KeyMutation.Builder builder = ValueProtos.KeyMutation.newBuilder();

for (Map.Entry<String, Entity> level0 : entity.getMap().entrySet()) {
switch (level0.getKey()) {
case RocksRef.COMMITS_KEY_ADDITION:
builder.setType(ValueProtos.KeyMutation.MutationType.ADDITION);
builder.setKey(entityToKey(level0.getValue()));
break;
case RocksRef.COMMITS_KEY_REMOVAL:
builder.setType(ValueProtos.KeyMutation.MutationType.REMOVAL);
builder.setKey(entityToKey(level0.getValue()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

builder.setKey() seems like it should be moved out of the switch given that it's common.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moved.

break;
default:
throw new UnsupportedOperationException();
}
}
return builder.build();
}

/**
* This converts an Entity into Key protobuf object.
* @param entity the entity to convert
* @return the object converted from the entity
*/
public static ValueProtos.Key entityToKey(Entity entity) {
if (!entity.getType().equals(Entity.EntityType.LIST)) {
throw new UnsupportedOperationException();
}

return ValueProtos.Key.newBuilder().addAllElements(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than collect into an intermediate list, consider using .forEach() to add as you process the stream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated everywhere that I could find.

entity.getList().stream().map(Entity::getString).collect(Collectors.toList())).build();
}

/**
* This converts an Entity into KeyDelta protobuf object.
* @param entity the entity to convert
* @return the object converted from the entity
*/
public static ValueProtos.KeyDelta entityToKeyDelta(Entity entity) {
if (entity.getType() != Entity.EntityType.MAP) {
throw new UnsupportedOperationException("Invalid value for keyDelta");
}

final Map<String, Entity> entityMap = entity.getMap();
final ValueProtos.KeyDelta.Builder keyDeltaBuilder = ValueProtos.KeyDelta.newBuilder();

for (Map.Entry<String, Entity> entry : entityMap.entrySet()) {
switch (entry.getKey()) {
case RocksL3.TREE_ID:
keyDeltaBuilder.setId(entry.getValue().getBinary());
break;
case RocksL3.TREE_KEY:
keyDeltaBuilder.setKey(entityToKey(entry.getValue()));
break;
default:
throw new UnsupportedOperationException(String.format("Unknown field \"%s\" for keyDelta", entry.getKey()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this the only exception which has an error message in this class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added messages to the other exceptions.

}
}

return keyDeltaBuilder.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright (C) 2020 Dremio
*
* Licensed 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.projectnessie.versioned.rocksdb;

import java.util.ArrayList;
import java.util.List;

import org.projectnessie.versioned.impl.condition.ExpressionPath;

class PathPattern {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this class for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A path is a nested structure, with each level wrapped in an optional. Parsing the path directly where it is used was getting really ugly, the deeper the path structure was. This makes it simpler to match a path against a pattern. An example would be to see if the path is fo a key in a keyMutation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense, there's a visitor pattern already in the Path, would it be possible to reuse that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be altered to implement ValueVisitor. I'll try that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now PathPattern implements ValueVisitor and is used using the Visitor pattern.

private List<java.util.function.Function<ExpressionPath.PathSegment, Boolean>> pathPatternElements = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

final?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated.


public PathPattern nameEquals(String name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why public?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No reason, changed to package.

pathPatternElements.add((p) -> p != null && p.isName() && name.equals(p.asName().getName()));
return this;
}

public PathPattern anyName() {
pathPatternElements.add((p) -> p != null && p.isName());
return this;
}

public PathPattern positionEquals(int position) {
pathPatternElements.add((p) -> p != null && p.isPosition() && position == p.asPosition().getPosition());
return this;
}

public PathPattern anyPosition() {
pathPatternElements.add((p) -> p != null && p.isPosition());
return this;
}

public boolean matches(ExpressionPath.PathSegment path) {
for (java.util.function.Function<ExpressionPath.PathSegment, Boolean> pathPatternElement : pathPatternElements) {
if (!pathPatternElement.apply(path)) {
return false;
} else if (path == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be switched such that the null check is first?

return false;
}

path = path.getChild().orElse(null);
}

return path == null;
}
}
Loading