Rocks condition expr rebase #19

stevelorddremio · 2021-02-13T05:48:31Z

This is equivalent to the external PR that will be posted to project-nessie with RocksDB rebase and condition expression handling.

kylepbit · 2021-02-14T02:27:12Z

versioned/tiered/mongodb/pom.xml

@@ -35,12 +35,6 @@
      <artifactId>nessie-versioned-tiered-impl</artifactId>
      <version>${project.version}</version>
    </dependency>
-    <dependency>


Do you know why this was done? Given that this is a Rocks patch, we should probably only change Rocks related items unless it's clearly denoted.

I'm really not sure where this crept in. I have reverted this change.

kylepbit · 2021-02-14T02:28:00Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/Function.java

+ */
+@Immutable
+abstract class Function {
+  static final String EQUALS = "equals";


I thought @tifflhl changed this into an enum?

She did but that patch didn't get in before I did the refactor. Should I apply her change to this branch?

I'm applying this change.

kylepbit · 2021-02-14T02:28:59Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+      final ExpressionPath.PathSegment pathSegment = function.getPath().getRoot().getChild().orElse(null);
+      if (pathSegment == null) {
+        return toEntity(stream).equals(function.getValue());
+      } else { // compare complete list


Can collapse this into an else if

Yes, collapsed.

kylepbit · 2021-02-14T02:29:26Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+   * @param function the condition to check
+   * @return true if the condition is met
+   */
+  public abstract boolean evaluateFunction(Function function);


Any reason not to just call this evaluate() ?

Agreed to just have an overloaded method name.

kylepbit · 2021-02-14T02:29:56Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+   * @param function the function to evaluate against id.
+   * @return true if the id meets the function condition
+   */
+  boolean idEvaluates(Function function) {


You'll probably get a comment about the name here, but I don't know a better name.

Maybe to swap the name around evaluatesId ?

Sure, please do so.

kylepbit · 2021-02-14T02:35:06Z

...ed/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksDBValueVisitor.java

+  }
+
+  @Override
+  public Function visit(ExpressionFunction value) {


I bet we'll get comments on the fact that the visitors really only have a single case for both. It does make me wonder if we should use the visitors at all, since you already know what you're dealing with?

I think it is worth waiting then refactoring if there are comments, and there will be comments.

kylepbit · 2021-02-14T02:35:28Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksFragment.java

+        if (function.getRootPathAsNameSegment().getChild().isPresent()) {
+          return false;
+        }
+        if (function.getOperator().equals(Function.EQUALS)) {


Why not using else if here, like below?

I see your point. else if it is.

kylepbit · 2021-02-14T02:36:37Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksRef.java

+      } else if (identifier.equals("t")) {
+        return TAG;
+      } else {
+        throw new IllegalArgumentException(String.format("Unknown identifier name [%s].", identifier));


Should this say: Unknown Type name?

Yes, except I'll keep Type lowercase.

kylepbit · 2021-02-14T02:37:00Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksRef.java

+      throw new IllegalStateException("branch()/tag() has already been called");
+    }
+    type = Type.TAG;
+    // TODO: Do we need to serialize ref type?


@stevelorddremio should this still be here?

No. Removed.

kylepbit · 2021-02-14T02:38:35Z

...ioned/tiered/tiered-tests/src/main/java/org/projectnessie/versioned/impl/SampleEntities.java

    return random.ints('a', 'z' + 1)
        .limit(numChars)
        .collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append)
        .toString();
  }
+


Can you revert the move of createStringEntity?

…ition. Thrown exception did not match API description.

codecov-io · 2021-02-14T10:08:22Z

Codecov Report

Merging #19 (47c23bb) into main (bf8994c) will decrease coverage by 14.83%.
The diff coverage is 75.91%.

@@              Coverage Diff              @@
##               main      #19       +/-   ##
=============================================
- Coverage     84.73%   69.89%   -14.84%     
- Complexity        0      294      +294     
=============================================
  Files            11      229      +218     
  Lines           871     9047     +8176     
  Branches        124      878      +754     
=============================================
+ Hits            738     6323     +5585     
- Misses           93     2261     +2168     
- Partials         40      463      +423

Flag	Coverage Δ	Complexity Δ
java	`68.30% <75.91%> (?)`	`0.00 <171.00> (?)`
python	`84.73% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...rojectnessie/versioned/mongodb/MongoBaseValue.java	`88.63% <ø> (ø)`	`15.00 <0.00> (?)`
.../org/projectnessie/versioned/rocksdb/Function.java	`37.50% <37.50%> (ø)`	`4.00 <4.00> (?)`
...rg/projectnessie/versioned/rocksdb/RocksValue.java	`44.44% <44.44%> (ø)`	`3.00 <3.00> (?)`
...ctnessie/versioned/rocksdb/RocksDBStoreConfig.java	`50.00% <50.00%> (ø)`	`1.00 <1.00> (?)`
...tnessie/versioned/rocksdb/RocksCommitMetadata.java	`66.66% <66.66%> (ø)`	`5.00 <5.00> (?)`
...projectnessie/versioned/rocksdb/RocksFragment.java	`68.96% <68.96%> (ø)`	`8.00 <8.00> (?)`
...tnessie/versioned/rocksdb/RocksDBValueVisitor.java	`70.83% <70.83%> (ø)`	`8.00 <8.00> (?)`
...rojectnessie/versioned/rocksdb/RocksBaseValue.java	`73.58% <73.58%> (ø)`	`24.00 <24.00> (?)`
.../projectnessie/versioned/rocksdb/RocksDBStore.java	`74.17% <74.17%> (ø)`	`41.00 <41.00> (?)`
...a/org/projectnessie/versioned/rocksdb/RocksL1.java	`78.26% <78.26%> (ø)`	`17.00 <17.00> (?)`
... and 224 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf8994c...47c23bb. Read the comment docs.

kylepbit · 2021-02-15T03:24:27Z

...iered/dynamodb/src/test/java/org/projectnessie/versioned/impl/condition/TestExpressions.java

    assertEquals("list_append(p0, :v0)", v0p.asString());
    assertEquals(AttributeValueUtil.fromEntity(av0), c.getAttributesValues().get(":v0").l().get(0));
  }

-  @Test


Finally figured out why this is removed. It's odd that this is removed here when AddClause isn't removed in this patch but by a different one, I think you should revert changes to this file.

Reverted back to main version

kylepbit · 2021-02-15T03:25:07Z

...ioned/tiered/mongodb/src/test/java/org/projectnessie/versioned/mongodb/TestMongoDBStore.java

@@ -28,4 +30,144 @@
  protected long getRandomSeed() {
    return 8612341233543L;
  }
+
+  @Test


Do you know why these are here when nothing has changed for MongoDB Store? I think the changes to this file should all be removed.

This is because I merged in the putConditional and deleteConditional tests to AbstractTestStore in the Rocks ConditionExpression branches. As mongo does not have this implemented in my branch I update TestMongoDBStore to disable these test. I see you the AbstractTestStore patch merged to main subsequently nested the AbstractTestStore tests and disabled the parent tests for mongo. So reverting this change in my branch brings it in line with that design approach.

kylepbit · 2021-02-15T03:26:37Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+   * @param function the function to evaluate against id.
+   * @return true if the id meets the function condition
+   */
+  boolean idEvaluates(Function function) {


Sure, please do so.

kylepbit · 2021-02-15T03:28:03Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksDBStore.java

+      final byte[] value = getAndCheckValue(transaction, columnFamilyHandle, id, operation);
+      final RocksBaseValue<C> consumer = RocksSerDe.getConsumer(type);
+      RocksSerDe.deserializeToConsumer(type, value, consumer);
+      if (!(consumer.evaluate(translate(condition.get())))) {


Nit: remove the extra braces here.

…ible. Refactored evaluate() in RocksRef.

…-rebase

stevelorddremio added 5 commits February 12, 2021 15:03

rebase on main

647c18a

Rocks package renames

a4ece26

Rocks checkstyle

311b3b8

Rocks checkstyle

f3e876c

Rocks checkstyle

0f256b4

kylepbit reviewed Feb 14, 2021

View reviewed changes

stevelorddremio added 2 commits February 14, 2021 01:23

Rocks PR updates. Correct AbstractTestStore put test for failing cond…

1fe009b

…ition. Thrown exception did not match API description.

RocksDB remove static declaration of Immutable builder() method.

47c23bb

kylepbit reviewed Feb 15, 2021

View reviewed changes

stevelorddremio and others added 17 commits February 15, 2021 00:23

Rocks PR updates. Revert mongodb changes.

7e1b772

RocksDB Ext PR updates

ab98b95

RocksDB ext PR updates

2b3705c

Rocks ext PR updates

a0df308

Rocks ext PR change

e68e7c5

RocksDB ext PR remove RocksDBStoreConfig default DbDirectory

af7e6e9

Rocks PR updates

0bc8225

RocksDB checkstyle

743e4d3

Switched to storing internal data using protocol buffer objects.

2a47d8d

Fixed an issue causing some failing unit tests.

e0e6108

Cleaned up the protocol buffer variable names. Added final where poss…

3b07f83

…ible. Refactored evaluate() in RocksRef.

RocksDB PR update. Remove unnecessary catch and throw of same exception.

96698d6

Fixed a checkstyle violation.

a660772

Merge branch 'rocks-condition-expr-normanj' into rocks-condition-expr…

81da271

…-rebase

RocksDB move deleteAllData() to test class

5f4006d

Address remaining code review comments.

e9bb750

RocksDB PR updates for ConditionExpression

b326565

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rocks condition expr rebase #19

Rocks condition expr rebase #19

stevelorddremio commented Feb 13, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 15, 2021

stevelorddremio Feb 15, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021 •

edited

Loading

kylepbit Feb 14, 2021

stevelorddremio Feb 14, 2021

codecov-io commented Feb 14, 2021

kylepbit Feb 15, 2021

stevelorddremio Feb 15, 2021

kylepbit Feb 15, 2021

stevelorddremio Feb 15, 2021 •

edited

Loading

kylepbit Feb 15, 2021

kylepbit Feb 15, 2021

stevelorddremio Feb 15, 2021

Rocks condition expr rebase #19

Are you sure you want to change the base?

Rocks condition expr rebase #19

Conversation

stevelorddremio commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelorddremio Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 14, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelorddremio Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelorddremio Feb 14, 2021 •

edited

Loading

stevelorddremio Feb 15, 2021 •

edited

Loading