You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jackrabbit.apache.org by ju...@apache.org on 2012/03/07 17:00:49 UTC
svn commit: r1298002 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Author: jukka
Date: Wed Mar 7 16:00:48 2012
New Revision: 1298002
URL: http://svn.apache.org/viewvc?rev=1298002&view=rev
Log:
OAK-3: Internal tree model
Use the abstract base classes and add an optimized equals() implementation based on the content id
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java?rev=1298002&r1=1298001&r2=1298002&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java Wed Mar 7 16:00:48 2012
@@ -21,11 +21,14 @@ import java.util.Iterator;
import java.util.Map;
import org.apache.jackrabbit.mk.model.StoredNode;
+import org.apache.jackrabbit.oak.model.AbstractChildNodeEntry;
+import org.apache.jackrabbit.oak.model.AbstractNodeState;
+import org.apache.jackrabbit.oak.model.AbstractPropertyState;
import org.apache.jackrabbit.oak.model.ChildNodeEntry;
import org.apache.jackrabbit.oak.model.NodeState;
import org.apache.jackrabbit.oak.model.PropertyState;
-class StoredNodeAsState implements NodeState {
+class StoredNodeAsState extends AbstractNodeState {
private final StoredNode node;
@@ -36,7 +39,7 @@ class StoredNodeAsState implements NodeS
this.provider = provider;
}
- private static class SimplePropertyState implements PropertyState {
+ private static class SimplePropertyState extends AbstractPropertyState {
private final String name;
@@ -47,16 +50,19 @@ class StoredNodeAsState implements NodeS
this.value = value;
}
+ @Override
public String getName() {
return name;
}
+ @Override
public String getEncodedValue() {
return value;
}
}
+ @Override
public PropertyState getProperty(String name) {
String value = node.getProperties().get(name);
if (value != null) {
@@ -66,10 +72,12 @@ class StoredNodeAsState implements NodeS
}
}
+ @Override
public long getPropertyCount() {
return node.getProperties().size();
}
+ @Override
public Iterable<PropertyState> getProperties() {
return new Iterable<PropertyState>() {
public Iterator<PropertyState> iterator() {
@@ -92,6 +100,7 @@ class StoredNodeAsState implements NodeS
};
}
+ @Override
public NodeState getChildNode(String name) {
org.apache.jackrabbit.mk.model.ChildNodeEntry entry =
node.getChildNodeEntry(name);
@@ -102,10 +111,12 @@ class StoredNodeAsState implements NodeS
}
}
+ @Override
public long getChildNodeCount() {
return node.getChildNodeCount();
}
+ @Override
public Iterable<ChildNodeEntry> getChildNodeEntries(
final long offset, final long length) {
if (length < -1) {
@@ -139,7 +150,7 @@ class StoredNodeAsState implements NodeS
private ChildNodeEntry getChildNodeEntry(
final org.apache.jackrabbit.mk.model.ChildNodeEntry entry) {
- return new ChildNodeEntry() {
+ return new AbstractChildNodeEntry() {
public String getName() {
return entry.getName();
}
@@ -154,4 +165,16 @@ class StoredNodeAsState implements NodeS
};
}
+ @Override
+ public boolean equals(Object that) {
+ if (that instanceof StoredNodeAsState) {
+ StoredNodeAsState other = (StoredNodeAsState) that;
+ if (provider == other.provider
+ && node.getId().equals(other.node.getId())) {
+ return true;
+ }
+ }
+ return super.equals(that);
+ }
+
}
Re: svn commit: r1298002 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
[s/dev@/oak-dev@/]
On Thu, Mar 8, 2012 at 12:47 AM, Michael Dürig <md...@apache.org> wrote:
> Right. I should have read the Javadoc ;-) However, I'd make it more explicit
> there, that sub classes must not refine equality (i.e. take into account
> values of other fields). This will in almost any case lead to a broken
> contract. For example (emphasis added): "Two property states are considered
> equal if *and only if* both their names and encoded values match."
Good point. Done in revision 1298833.
On a related note, we probably need to think about value encoding at
some point. Are we happy with having *all* property values persisted
as strings (or parsed/re-formatted at the storage layer), or should we
allow native formatting at least for basic types like numbers and
booleans? A related question is handling of value arrays for
multivalued properties.
BR,
Jukka Zitting
Re: svn commit: r1298002 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Posted by Michael Dürig <md...@apache.org>.
> On Wed, Mar 7, 2012 at 9:45 PM, Michael Dürig<mi...@gmail.com> wrote:
>> Regarding equals in the abstract base classes: I'd make the equals methods
>> in the abstract base classes final.
>
> No, the current non-final implementation is by design.
>
> See the followup patch I attached to OAK-3 for what I'm aiming at
> here. Basically I want to decouple the task of diffing node states
> from the underlying storage model (so far the diff() method has been
> an integral part of the .mk.model.Node interface).
>
> To do that efficiently, we need a fast way to compare potentially huge
> subtrees for equality. The underlying storage model can do that for
> example when the states point to the same storage location or have the
> same content hash, which is why they should be able to override the
> equals() method. Otherwise an equals() check could end up traversing
> the entire subtree.
Ok makes sense.
>> Overriding them in a concrete subclass will most likely break symmetry.
>
> If it does, it breaks the API contract and should be fixed.
Right. I should have read the Javadoc ;-) However, I'd make it more
explicit there, that sub classes must not refine equality (i.e. take
into account values of other fields). This will in almost any case lead
to a broken contract. For example (emphasis added): "Two property states
are considered equal if *and only if* both their names and encoded
values match."
Michael
Re: svn commit: r1298002 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Mar 7, 2012 at 9:45 PM, Michael Dürig <mi...@gmail.com> wrote:
> Regarding equals in the abstract base classes: I'd make the equals methods
> in the abstract base classes final.
No, the current non-final implementation is by design.
See the followup patch I attached to OAK-3 for what I'm aiming at
here. Basically I want to decouple the task of diffing node states
from the underlying storage model (so far the diff() method has been
an integral part of the .mk.model.Node interface).
To do that efficiently, we need a fast way to compare potentially huge
subtrees for equality. The underlying storage model can do that for
example when the states point to the same storage location or have the
same content hash, which is why they should be able to override the
equals() method. Otherwise an equals() check could end up traversing
the entire subtree.
> Overriding them in a concrete subclass will most likely break symmetry.
If it does, it breaks the API contract and should be fixed.
BR,
Jukka Zitting
Re: svn commit: r1298002 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Posted by Michael Dürig <mi...@gmail.com>.
Hi,
Regarding equals in the abstract base classes: I'd make the equals
methods in the abstract base classes final. Overriding them in a
concrete subclass will most likely break symmetry. Furthermore, since
these classes are immutable, the result of equals can be cached safely.
Michael
On 7.3.12 16:00, jukka@apache.org wrote:
> Author: jukka
> Date: Wed Mar 7 16:00:48 2012
> New Revision: 1298002
>
> URL: http://svn.apache.org/viewvc?rev=1298002&view=rev
> Log:
> OAK-3: Internal tree model
>
> Use the abstract base classes and add an optimized equals() implementation based on the content id
>
> Modified:
> jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
>
> Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java?rev=1298002&r1=1298001&r2=1298002&view=diff
> ==============================================================================
> --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java (original)
> +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java Wed Mar 7 16:00:48 2012
> @@ -21,11 +21,14 @@ import java.util.Iterator;
> import java.util.Map;
>
> import org.apache.jackrabbit.mk.model.StoredNode;
> +import org.apache.jackrabbit.oak.model.AbstractChildNodeEntry;
> +import org.apache.jackrabbit.oak.model.AbstractNodeState;
> +import org.apache.jackrabbit.oak.model.AbstractPropertyState;
> import org.apache.jackrabbit.oak.model.ChildNodeEntry;
> import org.apache.jackrabbit.oak.model.NodeState;
> import org.apache.jackrabbit.oak.model.PropertyState;
>
> -class StoredNodeAsState implements NodeState {
> +class StoredNodeAsState extends AbstractNodeState {
>
> private final StoredNode node;
>
> @@ -36,7 +39,7 @@ class StoredNodeAsState implements NodeS
> this.provider = provider;
> }
>
> - private static class SimplePropertyState implements PropertyState {
> + private static class SimplePropertyState extends AbstractPropertyState {
>
> private final String name;
>
> @@ -47,16 +50,19 @@ class StoredNodeAsState implements NodeS
> this.value = value;
> }
>
> + @Override
> public String getName() {
> return name;
> }
>
> + @Override
> public String getEncodedValue() {
> return value;
> }
>
> }
>
> + @Override
> public PropertyState getProperty(String name) {
> String value = node.getProperties().get(name);
> if (value != null) {
> @@ -66,10 +72,12 @@ class StoredNodeAsState implements NodeS
> }
> }
>
> + @Override
> public long getPropertyCount() {
> return node.getProperties().size();
> }
>
> + @Override
> public Iterable<PropertyState> getProperties() {
> return new Iterable<PropertyState>() {
> public Iterator<PropertyState> iterator() {
> @@ -92,6 +100,7 @@ class StoredNodeAsState implements NodeS
> };
> }
>
> + @Override
> public NodeState getChildNode(String name) {
> org.apache.jackrabbit.mk.model.ChildNodeEntry entry =
> node.getChildNodeEntry(name);
> @@ -102,10 +111,12 @@ class StoredNodeAsState implements NodeS
> }
> }
>
> + @Override
> public long getChildNodeCount() {
> return node.getChildNodeCount();
> }
>
> + @Override
> public Iterable<ChildNodeEntry> getChildNodeEntries(
> final long offset, final long length) {
> if (length< -1) {
> @@ -139,7 +150,7 @@ class StoredNodeAsState implements NodeS
>
> private ChildNodeEntry getChildNodeEntry(
> final org.apache.jackrabbit.mk.model.ChildNodeEntry entry) {
> - return new ChildNodeEntry() {
> + return new AbstractChildNodeEntry() {
> public String getName() {
> return entry.getName();
> }
> @@ -154,4 +165,16 @@ class StoredNodeAsState implements NodeS
> };
> }
>
> + @Override
> + public boolean equals(Object that) {
> + if (that instanceof StoredNodeAsState) {
> + StoredNodeAsState other = (StoredNodeAsState) that;
> + if (provider == other.provider
> +&& node.getId().equals(other.node.getId())) {
> + return true;
> + }
> + }
> + return super.equals(that);
> + }
> +
> }
>
>
Re: svn commit: r1298002 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
Posted by Michael Dürig <md...@apache.org>.
Hi,
Regarding equals in the abstract base classes: I'd make the equals
methods in the abstract base classes final. Overriding them in a
concrete subclass will most likely break symmetry. Furthermore, since
these classes are immutable, the result of equals can be cached safely.
Michael
On 7.3.12 16:00, jukka@apache.org wrote:
> Author: jukka
> Date: Wed Mar 7 16:00:48 2012
> New Revision: 1298002
>
> URL: http://svn.apache.org/viewvc?rev=1298002&view=rev
> Log:
> OAK-3: Internal tree model
>
> Use the abstract base classes and add an optimized equals() implementation based on the content id
>
> Modified:
> jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
>
> Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java?rev=1298002&r1=1298001&r2=1298002&view=diff
> ==============================================================================
> --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java (original)
> +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/mk/store/StoredNodeAsState.java Wed Mar 7 16:00:48 2012
> @@ -21,11 +21,14 @@ import java.util.Iterator;
> import java.util.Map;
>
> import org.apache.jackrabbit.mk.model.StoredNode;
> +import org.apache.jackrabbit.oak.model.AbstractChildNodeEntry;
> +import org.apache.jackrabbit.oak.model.AbstractNodeState;
> +import org.apache.jackrabbit.oak.model.AbstractPropertyState;
> import org.apache.jackrabbit.oak.model.ChildNodeEntry;
> import org.apache.jackrabbit.oak.model.NodeState;
> import org.apache.jackrabbit.oak.model.PropertyState;
>
> -class StoredNodeAsState implements NodeState {
> +class StoredNodeAsState extends AbstractNodeState {
>
> private final StoredNode node;
>
> @@ -36,7 +39,7 @@ class StoredNodeAsState implements NodeS
> this.provider = provider;
> }
>
> - private static class SimplePropertyState implements PropertyState {
> + private static class SimplePropertyState extends AbstractPropertyState {
>
> private final String name;
>
> @@ -47,16 +50,19 @@ class StoredNodeAsState implements NodeS
> this.value = value;
> }
>
> + @Override
> public String getName() {
> return name;
> }
>
> + @Override
> public String getEncodedValue() {
> return value;
> }
>
> }
>
> + @Override
> public PropertyState getProperty(String name) {
> String value = node.getProperties().get(name);
> if (value != null) {
> @@ -66,10 +72,12 @@ class StoredNodeAsState implements NodeS
> }
> }
>
> + @Override
> public long getPropertyCount() {
> return node.getProperties().size();
> }
>
> + @Override
> public Iterable<PropertyState> getProperties() {
> return new Iterable<PropertyState>() {
> public Iterator<PropertyState> iterator() {
> @@ -92,6 +100,7 @@ class StoredNodeAsState implements NodeS
> };
> }
>
> + @Override
> public NodeState getChildNode(String name) {
> org.apache.jackrabbit.mk.model.ChildNodeEntry entry =
> node.getChildNodeEntry(name);
> @@ -102,10 +111,12 @@ class StoredNodeAsState implements NodeS
> }
> }
>
> + @Override
> public long getChildNodeCount() {
> return node.getChildNodeCount();
> }
>
> + @Override
> public Iterable<ChildNodeEntry> getChildNodeEntries(
> final long offset, final long length) {
> if (length< -1) {
> @@ -139,7 +150,7 @@ class StoredNodeAsState implements NodeS
>
> private ChildNodeEntry getChildNodeEntry(
> final org.apache.jackrabbit.mk.model.ChildNodeEntry entry) {
> - return new ChildNodeEntry() {
> + return new AbstractChildNodeEntry() {
> public String getName() {
> return entry.getName();
> }
> @@ -154,4 +165,16 @@ class StoredNodeAsState implements NodeS
> };
> }
>
> + @Override
> + public boolean equals(Object that) {
> + if (that instanceof StoredNodeAsState) {
> + StoredNodeAsState other = (StoredNodeAsState) that;
> + if (provider == other.provider
> +&& node.getId().equals(other.node.getId())) {
> + return true;
> + }
> + }
> + return super.equals(that);
> + }
> +
> }
>
>