You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "GumpacG (via GitHub)" <gi...@apache.org> on 2026/06/03 22:41:09 UTC
[PR] Make Tree no longer extend HashMap [tinkerpop]
GumpacG opened a new pull request, #3448:
URL: https://github.com/apache/tinkerpop/pull/3448
## Summary
Changes `Tree` from `extends HashMap<T, Tree<T>>` to a `final` class that
holds a `Map` internally and exposes a tree-shaped public API. `Tree` is no
longer a `Map`, which removes inherited `Map` methods whose behavior did not
align with tree semantics (e.g. `size()` returned root-entry count, not total
nodes; `containsKey` only checked immediate children).
Per the dev-list discussion: https://lists.apache.org/thread/o0nqh6kmrkdht531655p351ldjll045d
This is a breaking change targeted at TinkerPop 4.0.0.
## Changes
`Tree` now exposes:
- Navigation: `rootNodes()`, `childAt(key)` (throws if absent), `hasChild(key)`,
`contains(value)` (recursive), `findSubtree(key)` (recursive, `Optional`),
`getOrCreateChild(key)` (construction primitive).
- Structure: `isLeaf()`, `nodeCount()`, `getNodesAtDepth(int)`,
`getTreesAtDepth(int)`, `getLeafNodes()`, `getLeafTrees()`.
- Composition/output: `addTree()`, `splitParents()`, `prettyPrint()`,
structural `equals`/`hashCode`/`toString`.
Behavior fixes that fall out of the rewrite:
- `isLeaf()` returns `true` on an empty tree instead of throwing.
- `getNodesAtDepth(0)` returns the root nodes (now 0-based).
Internal callers migrated off the removed `Map` surface:
- `TreeStep` / `TreeSideEffectStep` use `getOrCreateChild` during construction.
- GraphBinary `TreeSerializer` and GraphSON `TreeJacksonSerializer`/
`Deserializer` (V1–V4) iterate via `rootNodes()`/`childAt()` and rebuild via
`getOrCreateChild()` + `addTree()`.
- `DetachedFactory` / `ReferenceFactory` gained an explicit `Tree` branch
(previously handled via the `instanceof Map` path).
## Breaking changes
- Code treating a `Tree` as a `Map` (assignment to `Map`, `instanceof Map`,
`get`/`put`/`keySet`/`entrySet`/`size`) no longer compiles.
- Gremlin patterns that worked only because `Tree` was a `Map` (`select(keys)`,
`count(local)`, `unfold()` on a tree) no longer compose; process the result
client-side after `next()` or reshape the upstream traversal.
## Speed and Memory
Analytical estimates (not benchmarked); the backing `HashMap` table and entries
are identical before and after.
- **Memory:** ~16 B more per node — `Tree` now holds a `HashMap` instead of
being one, adding one wrapper object per node. The map itself is unchanged.
- **Speed:** comparable. Construction is slightly faster (`getOrCreateChild` is
1–2 map ops vs. the old 3); `childAt` is one op slower (collapsible to one).
Wire-format encoding is unchanged.
## Testing
- `TreeTest` rewritten against the new API (navigation, recursive
containment/search, depth queries, leaf detection, null keys, structural
equality, `toString`, pretty-print); `TreeSupplierTest` updated.
- `mvn -pl gremlin-core test -Dtest=TreeTest,TreeSupplierTest` passes.
Assisted-by: Kiro: Claude Opus 4.8
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399891315
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
Review Comment:
I agree that it's not exactly a "node" however, renaming to "objects" may make other tree functions not very tree-like. For example, `nodeCount()` would have to be `objectCount()` and `rootNodes()` would have to be `rootObjects()`. I think it would be more conducive for tree users to keep "node".
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365472136
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
+|`getLeafObjects()` |`getLeafNodes()`
+|=======================
+
+A few Gremlin patterns that worked only because `Tree` was a `Map` (for example `select(keys)`, `count(local)`,
+and `unfold()` applied to a `Tree`) no longer compose. Process the `Tree` result client-side after `next()`, or
+reshape the upstream traversal. The old `count(local)` (which returned the root-node count) is replaced by
+`rootNodes().size()` for the root count, or `nodeCount()` for the total node count.
+
+`isLeaf()` no longer throws on an empty tree (it returns `true`), and a `Tree` keeps the long-standing limitation
+that sibling branches resolving to the same value collapse into one node; use `path()` or `subgraph()` when full
+path structure must be preserved.
+
+
=== Upgrading for Providers
==== Graph System Providers
+`Tree` no longer extends `HashMap`. Provider code that inspected or rebuilt a `Tree` via `Map` methods
Review Comment:
It's also missing a section header.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399678608
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -5049,14 +5049,14 @@ It is important to see how the paths of all the emanating traversers are united
image::tree-step2.png[width=500]
-The resultant tree data structure can then be manipulated (see `Tree` JavaDoc).
+The resultant tree data structure can then be navigated with its tree-shaped API such as `childAt()`, `findSubtree()`, `getNodesAtDepth()`, and `getLeafNodes()` (see `Tree` JavaDoc).
[gremlin-groovy,modern]
----
tree = g.V().out().out().tree().by('name').next()
-tree['marko']
-tree['marko']['josh']
-tree.getObjectsAtDepth(3)
+tree.childAt('marko')
Review Comment:
I've updated it to include tree in the SugarLoader to include support the old syntax. Docs will just have the old syntax in the comment as an option with the sugar plugin.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365469053
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
+|`getLeafObjects()` |`getLeafNodes()`
+|=======================
+
+A few Gremlin patterns that worked only because `Tree` was a `Map` (for example `select(keys)`, `count(local)`,
+and `unfold()` applied to a `Tree`) no longer compose. Process the `Tree` result client-side after `next()`, or
+reshape the upstream traversal. The old `count(local)` (which returned the root-node count) is replaced by
+`rootNodes().size()` for the root count, or `nodeCount()` for the total node count.
+
+`isLeaf()` no longer throws on an empty tree (it returns `true`), and a `Tree` keeps the long-standing limitation
+that sibling branches resolving to the same value collapse into one node; use `path()` or `subgraph()` when full
+path structure must be preserved.
+
+
=== Upgrading for Providers
==== Graph System Providers
+`Tree` no longer extends `HashMap`. Provider code that inspected or rebuilt a `Tree` via `Map` methods
Review Comment:
This section assumes that the provider uses the reference deserializers. Providers can technically deserialize the tree bytes into whatever class they want. While this information is good as most providers will probably do that, it should probably more of a heads up to providers that do that they should expect changes.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399679893
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
+|`getLeafObjects()` |`getLeafNodes()`
+|=======================
+
+A few Gremlin patterns that worked only because `Tree` was a `Map` (for example `select(keys)`, `count(local)`,
+and `unfold()` applied to a `Tree`) no longer compose. Process the `Tree` result client-side after `next()`, or
+reshape the upstream traversal. The old `count(local)` (which returned the root-node count) is replaced by
+`rootNodes().size()` for the root count, or `nodeCount()` for the total node count.
+
+`isLeaf()` no longer throws on an empty tree (it returns `true`), and a `Tree` keeps the long-standing limitation
+that sibling branches resolving to the same value collapse into one node; use `path()` or `subgraph()` when full
+path structure must be preserved.
+
+
Review Comment:
Added, thanks!
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3389080167
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
Review Comment:
i know we wanted more tree terminology so we moved from "Objects" to "Node", but it just occurred to me that the return here isn't exactly a "node", is it? I wonder if that will be confusing.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3389063326
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -5049,14 +5049,14 @@ It is important to see how the paths of all the emanating traversers are united
image::tree-step2.png[width=500]
-The resultant tree data structure can then be manipulated (see `Tree` JavaDoc).
+The resultant tree data structure can then be navigated with its tree-shaped API such as `childAt()`, `findSubtree()`, `getNodesAtDepth()`, and `getLeafNodes()` (see `Tree` JavaDoc).
[gremlin-groovy,modern]
----
tree = g.V().out().out().tree().by('name').next()
-tree['marko']
-tree['marko']['josh']
-tree.getObjectsAtDepth(3)
+tree.childAt('marko')
Review Comment:
would be nice if we didn't lose the groovy syntax. `childAt` is better than the Java alternative but less so for Groovy. a bit of Groovy sugar would bring that back.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399664469
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/CountLocalStep.java:
##########
@@ -42,6 +43,9 @@ public CountLocalStep(final Traversal.Admin traversal) {
@Override
protected Long map(final Traverser.Admin<S> traverser) {
final S item = traverser.get();
+ if (item instanceof Tree)
Review Comment:
Tree used to return count of root nodes which is misleading. As per the [devlist post](https://lists.apache.org/thread/15lslfrs5f27hsz00k147mwc38o58w5k), it is not counting all nodes.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365926734
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/Tree.feature:
##########
@@ -206,36 +206,3 @@ Feature: Step - tree()
|--josh
|--peter
"""
-
- Scenario: g_V_out_treeXaX_selectXaX_countXlocalX
- Given the modern graph
- And the traversal of
- """
- g.V().out().tree("a").select("a").count(local)
Review Comment:
I guess these tests were broken as `tree().count(local)` was reliant on Tree being a map to give the count. I'd ideally like to avoid removing these tests. They weren't actually intended to test semantics of `tree().count(local)`, the pair of tests was meant to verify that TreeSideEffectStep acts as a barrier (processes all incoming traversers before producing a single result). Using `count(local)` to introspect the Tree from within gremlin was simply a nice convenience as `g.V().out().tree("a").select("a")` returns 6 references to the same Tree object, which is difficult for the tests to handle.
What are your thoughts on adding in a new case to `CountLocalStep` to handle `Tree`? It could simply call `rootNodes().size()` to retain the old semantics of counting the number of roots, although the more intuitive semantics would be if `tree().count(local)` mapped to the new `nodeCount()` method. I'm not sure how others may feel about a minor semantic break such as this.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365515179
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/CountLocalStep.java:
##########
@@ -42,6 +43,9 @@ public CountLocalStep(final Traversal.Admin traversal) {
@Override
protected Long map(final Traverser.Admin<S> traverser) {
final S item = traverser.get();
+ if (item instanceof Tree)
Review Comment:
Why specialize case Tree here? Wouldn't the default just return a singletonIterator so the count would be 1?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#issuecomment-4709667455
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3389091793
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -501,10 +501,49 @@ It cannot be passed to `traversal().with(...)`, and mutating its collections has
re-query elements against the original graph, extract their ids and call `g.V(id)` or `g.E(id)` against the original
`GraphTraversalSource`.
+==== Tree No Longer Extends HashMap
+
+`Tree` no longer extends `HashMap`. It is now a `final` class that holds a `Map` internally and exposes a
+tree-shaped API instead of `Map` methods. This is a breaking change for code that treated a `tree()` result as a
+`Map`.
+
+Replacements for the common `Map`-based access patterns:
+
+[options="header"]
+|=======================
+|3.x (`Tree` as `Map`) |4.x (`Tree` API)
+|`tree.get(key)` |`tree.childAt(key)` (throws if absent) or `tree.findSubtree(key)` (recursive, returns `Optional`)
+|`tree.containsKey(key)` |`tree.hasChild(key)`
+|`tree.keySet()` |`tree.rootNodes()`
+|`tree.size()` |`tree.rootNodes().size()` for root entries, or `tree.nodeCount()` for total nodes
+|`getObjectsAtDepth(d)` |`getNodesAtDepth(d)` (now 0-based: depth 0 returns the roots)
+|`getLeafObjects()` |`getLeafNodes()`
+|=======================
+
+A few Gremlin patterns that worked only because `Tree` was a `Map` (for example `select(keys)`, `count(local)`,
+and `unfold()` applied to a `Tree`) no longer compose. Process the `Tree` result client-side after `next()`, or
+reshape the upstream traversal. The old `count(local)` (which returned the root-node count) is replaced by
+`rootNodes().size()` for the root count, or `nodeCount()` for the total node count.
+
+`isLeaf()` no longer throws on an empty tree (it returns `true`), and a `Tree` keeps the long-standing limitation
+that sibling branches resolving to the same value collapse into one node; use `path()` or `subgraph()` when full
+path structure must be preserved.
+
+
Review Comment:
No JIRA for this right? We do have a DISCUSS thread though we could do a "See: " for further details.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3389120057
##########
CHANGELOG.asciidoc:
##########
@@ -25,6 +25,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
[[release-4-0-0]]
=== TinkerPop 4.0.0 (Release Date: NOT OFFICIALLY RELEASED YET)
+* Changed `Tree` to no longer extend `HashMap`; it is now a final class with a tree-shaped API (`childAt`, `hasChild`, `contains`, `findSubtree`, `getOrCreateChild`, `getNodesAtDepth`, `getLeafNodes`, `nodeCount`) and is no longer a `Map`.
Review Comment:
This entry is a bit long and detailed. Maybe "Changed `Tree` class in Java to not extend from `HashMap` and offered a new tree-shaped API for navigation."
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399671783
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/TreeTest.java:
##########
@@ -24,85 +24,233 @@
import org.hamcrest.Matchers;
import org.junit.Test;
-import java.util.AbstractMap;
import java.util.Arrays;
import java.util.List;
-import java.util.Map;
+import java.util.Optional;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
* @author Daniel Kuppitz (http://gremlin.guru)
*/
public class TreeTest extends StepTest {
+ /**
+ * Helper that builds a subtree shaped like {@code key -> subtree}.
+ */
+ private static <T> Tree<T> branch(final T key, final Tree<T> subtree) {
+ final Tree<T> wrapper = new Tree<>();
+ wrapper.getOrCreateChild(key).addTree(subtree);
+ return wrapper;
+ }
+
@Test
public void shouldProvideValidDepths() {
- Tree<String> tree = new Tree<String>();
- tree.put("marko", new Tree<String>(TreeTest.createTree("a", new Tree<String>("a1", "a2")), TreeTest.createTree("b", new Tree<String>("b1", "b2", "b3"))));
- tree.put("josh", new Tree<String>("1", "2"));
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> markoSubtree = new Tree<>();
+ markoSubtree.addTree(branch("a", new Tree<>("a1", "a2")));
+ markoSubtree.addTree(branch("b", new Tree<>("b1", "b2", "b3")));
+ tree.getOrCreateChild("marko").addTree(markoSubtree);
+ tree.getOrCreateChild("josh").addTree(new Tree<>("1", "2"));
+
+ assertEquals(2, tree.getNodesAtDepth(0).size());
+ assertTrue(tree.getNodesAtDepth(0).containsAll(Arrays.asList("marko", "josh")));
+ assertEquals(4, tree.getNodesAtDepth(1).size());
+ assertEquals(5, tree.getNodesAtDepth(2).size());
+ assertEquals(0, tree.getNodesAtDepth(3).size());
+ assertEquals(0, tree.getNodesAtDepth(4).size());
+ assertEquals(0, tree.getNodesAtDepth(5).size());
- assertEquals(0, tree.getObjectsAtDepth(0).size());
- assertEquals(2, tree.getObjectsAtDepth(1).size());
- assertEquals(4, tree.getObjectsAtDepth(2).size());
- assertEquals(5, tree.getObjectsAtDepth(3).size());
- assertEquals(0, tree.getObjectsAtDepth(4).size());
- assertEquals(0, tree.getObjectsAtDepth(5).size());
+ assertEquals(1, tree.getTreesAtDepth(0).size());
+ assertEquals(tree, tree.getTreesAtDepth(0).get(0));
- assertEquals(2, tree.get("josh").size());
- assertEquals(0, tree.get("marko").get("b").get("b1").size());
- assertEquals(3, tree.get("marko").get("b").size());
- assertNull(tree.get("marko").get("c"));
+ assertEquals(2, tree.childAt("josh").rootNodes().size());
+ assertTrue(tree.childAt("marko").childAt("b").childAt("b1").isLeaf());
+ assertEquals(3, tree.childAt("marko").childAt("b").rootNodes().size());
+ assertFalse(tree.childAt("marko").hasChild("c"));
}
@Test
public void shouldProvideValidLeaves() {
- Tree<String> tree = new Tree<String>();
- tree.put("marko", new Tree<String>(TreeTest.createTree("a", new Tree<String>("a1", "a2")), TreeTest.createTree("b", new Tree<String>("b1", "b2", "b3"))));
- tree.put("josh", new Tree<String>("1", "2"));
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> markoSubtree = new Tree<>();
+ markoSubtree.addTree(branch("a", new Tree<>("a1", "a2")));
+ markoSubtree.addTree(branch("b", new Tree<>("b1", "b2", "b3")));
+ tree.getOrCreateChild("marko").addTree(markoSubtree);
+ tree.getOrCreateChild("josh").addTree(new Tree<>("1", "2"));
assertEquals(7, tree.getLeafTrees().size());
- for (Tree<String> t : tree.getLeafTrees()) {
- assertEquals(1, t.keySet().size());
- final String key = t.keySet().iterator().next();
+ for (final Tree<String> t : tree.getLeafTrees()) {
+ assertEquals(1, t.rootNodes().size());
+ final String key = t.rootNodes().iterator().next();
assertTrue(Arrays.asList("a1", "a2", "b1", "b2", "b3", "1", "2").contains(key));
}
- assertEquals(7, tree.getLeafObjects().size());
- for (String s : tree.getLeafObjects()) {
+ assertEquals(7, tree.getLeafNodes().size());
+ for (final String s : tree.getLeafNodes()) {
assertTrue(Arrays.asList("a1", "a2", "b1", "b2", "b3", "1", "2").contains(s));
}
}
@Test
public void shouldMergeTreesCorrectly() {
- Tree<String> tree1 = new Tree<>();
- tree1.put("1", new Tree<String>(TreeTest.createTree("1_1", new Tree<String>("1_1_1")), TreeTest.createTree("1_2", new Tree<String>("1_2_1"))));
- Tree<String> tree2 = new Tree<>();
- tree2.put("1", new Tree<String>(TreeTest.createTree("1_1", new Tree<String>("1_1_1")), TreeTest.createTree("1_2", new Tree<String>("1_2_2"))));
+ final Tree<String> tree1 = new Tree<>();
+ final Tree<String> tree1OneSubtree = new Tree<>();
+ tree1OneSubtree.addTree(branch("1_1", new Tree<>("1_1_1")));
+ tree1OneSubtree.addTree(branch("1_2", new Tree<>("1_2_1")));
+ tree1.getOrCreateChild("1").addTree(tree1OneSubtree);
+
+ final Tree<String> tree2 = new Tree<>();
+ final Tree<String> tree2OneSubtree = new Tree<>();
+ tree2OneSubtree.addTree(branch("1_1", new Tree<>("1_1_1")));
+ tree2OneSubtree.addTree(branch("1_2", new Tree<>("1_2_2")));
+ tree2.getOrCreateChild("1").addTree(tree2OneSubtree);
- Tree<String> mergeTree = new Tree<>();
+ final Tree<String> mergeTree = new Tree<>();
mergeTree.addTree(tree1);
mergeTree.addTree(tree2);
- assertEquals(1, mergeTree.size());
- assertEquals(0, mergeTree.getObjectsAtDepth(0).size());
- assertEquals(1, mergeTree.getObjectsAtDepth(1).size());
- assertEquals(2, mergeTree.getObjectsAtDepth(2).size());
- assertEquals(3, mergeTree.getObjectsAtDepth(3).size());
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_1_1"));
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_2_1"));
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_2_2"));
+ assertEquals(1, mergeTree.rootNodes().size());
+ assertEquals(1, mergeTree.getNodesAtDepth(0).size());
+ assertEquals(2, mergeTree.getNodesAtDepth(1).size());
+ assertEquals(3, mergeTree.getNodesAtDepth(2).size());
+ assertEquals(0, mergeTree.getNodesAtDepth(3).size());
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_1_1"));
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_2_1"));
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_2_2"));
+ }
+
+ @Test
+ public void shouldGetOrCreateChild() {
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> child = tree.getOrCreateChild("a");
+ assertNotNull(child);
+ assertTrue(child.isLeaf());
+ // calling again returns the same instance
+ assertSame(child, tree.getOrCreateChild("a"));
+
+ // mutating the returned subtree is observable through the parent
+ child.getOrCreateChild("a1");
+ assertTrue(tree.childAt("a").hasChild("a1"));
+ }
+
+ @Test
+ public void shouldAllowNullKeys() {
Review Comment:
Null will be allowed as removing it could remove valuable paths in the tree. If for example, it won't be possible in .NET, then it would be a documented limitation or have an implementation that is not in the default library of .NET.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365586031
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/TreeTest.java:
##########
@@ -24,85 +24,233 @@
import org.hamcrest.Matchers;
import org.junit.Test;
-import java.util.AbstractMap;
import java.util.Arrays;
import java.util.List;
-import java.util.Map;
+import java.util.Optional;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
* @author Daniel Kuppitz (http://gremlin.guru)
*/
public class TreeTest extends StepTest {
+ /**
+ * Helper that builds a subtree shaped like {@code key -> subtree}.
+ */
+ private static <T> Tree<T> branch(final T key, final Tree<T> subtree) {
+ final Tree<T> wrapper = new Tree<>();
+ wrapper.getOrCreateChild(key).addTree(subtree);
+ return wrapper;
+ }
+
@Test
public void shouldProvideValidDepths() {
- Tree<String> tree = new Tree<String>();
- tree.put("marko", new Tree<String>(TreeTest.createTree("a", new Tree<String>("a1", "a2")), TreeTest.createTree("b", new Tree<String>("b1", "b2", "b3"))));
- tree.put("josh", new Tree<String>("1", "2"));
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> markoSubtree = new Tree<>();
+ markoSubtree.addTree(branch("a", new Tree<>("a1", "a2")));
+ markoSubtree.addTree(branch("b", new Tree<>("b1", "b2", "b3")));
+ tree.getOrCreateChild("marko").addTree(markoSubtree);
+ tree.getOrCreateChild("josh").addTree(new Tree<>("1", "2"));
+
+ assertEquals(2, tree.getNodesAtDepth(0).size());
+ assertTrue(tree.getNodesAtDepth(0).containsAll(Arrays.asList("marko", "josh")));
+ assertEquals(4, tree.getNodesAtDepth(1).size());
+ assertEquals(5, tree.getNodesAtDepth(2).size());
+ assertEquals(0, tree.getNodesAtDepth(3).size());
+ assertEquals(0, tree.getNodesAtDepth(4).size());
+ assertEquals(0, tree.getNodesAtDepth(5).size());
- assertEquals(0, tree.getObjectsAtDepth(0).size());
- assertEquals(2, tree.getObjectsAtDepth(1).size());
- assertEquals(4, tree.getObjectsAtDepth(2).size());
- assertEquals(5, tree.getObjectsAtDepth(3).size());
- assertEquals(0, tree.getObjectsAtDepth(4).size());
- assertEquals(0, tree.getObjectsAtDepth(5).size());
+ assertEquals(1, tree.getTreesAtDepth(0).size());
+ assertEquals(tree, tree.getTreesAtDepth(0).get(0));
- assertEquals(2, tree.get("josh").size());
- assertEquals(0, tree.get("marko").get("b").get("b1").size());
- assertEquals(3, tree.get("marko").get("b").size());
- assertNull(tree.get("marko").get("c"));
+ assertEquals(2, tree.childAt("josh").rootNodes().size());
+ assertTrue(tree.childAt("marko").childAt("b").childAt("b1").isLeaf());
+ assertEquals(3, tree.childAt("marko").childAt("b").rootNodes().size());
+ assertFalse(tree.childAt("marko").hasChild("c"));
}
@Test
public void shouldProvideValidLeaves() {
- Tree<String> tree = new Tree<String>();
- tree.put("marko", new Tree<String>(TreeTest.createTree("a", new Tree<String>("a1", "a2")), TreeTest.createTree("b", new Tree<String>("b1", "b2", "b3"))));
- tree.put("josh", new Tree<String>("1", "2"));
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> markoSubtree = new Tree<>();
+ markoSubtree.addTree(branch("a", new Tree<>("a1", "a2")));
+ markoSubtree.addTree(branch("b", new Tree<>("b1", "b2", "b3")));
+ tree.getOrCreateChild("marko").addTree(markoSubtree);
+ tree.getOrCreateChild("josh").addTree(new Tree<>("1", "2"));
assertEquals(7, tree.getLeafTrees().size());
- for (Tree<String> t : tree.getLeafTrees()) {
- assertEquals(1, t.keySet().size());
- final String key = t.keySet().iterator().next();
+ for (final Tree<String> t : tree.getLeafTrees()) {
+ assertEquals(1, t.rootNodes().size());
+ final String key = t.rootNodes().iterator().next();
assertTrue(Arrays.asList("a1", "a2", "b1", "b2", "b3", "1", "2").contains(key));
}
- assertEquals(7, tree.getLeafObjects().size());
- for (String s : tree.getLeafObjects()) {
+ assertEquals(7, tree.getLeafNodes().size());
+ for (final String s : tree.getLeafNodes()) {
assertTrue(Arrays.asList("a1", "a2", "b1", "b2", "b3", "1", "2").contains(s));
}
}
@Test
public void shouldMergeTreesCorrectly() {
- Tree<String> tree1 = new Tree<>();
- tree1.put("1", new Tree<String>(TreeTest.createTree("1_1", new Tree<String>("1_1_1")), TreeTest.createTree("1_2", new Tree<String>("1_2_1"))));
- Tree<String> tree2 = new Tree<>();
- tree2.put("1", new Tree<String>(TreeTest.createTree("1_1", new Tree<String>("1_1_1")), TreeTest.createTree("1_2", new Tree<String>("1_2_2"))));
+ final Tree<String> tree1 = new Tree<>();
+ final Tree<String> tree1OneSubtree = new Tree<>();
+ tree1OneSubtree.addTree(branch("1_1", new Tree<>("1_1_1")));
+ tree1OneSubtree.addTree(branch("1_2", new Tree<>("1_2_1")));
+ tree1.getOrCreateChild("1").addTree(tree1OneSubtree);
+
+ final Tree<String> tree2 = new Tree<>();
+ final Tree<String> tree2OneSubtree = new Tree<>();
+ tree2OneSubtree.addTree(branch("1_1", new Tree<>("1_1_1")));
+ tree2OneSubtree.addTree(branch("1_2", new Tree<>("1_2_2")));
+ tree2.getOrCreateChild("1").addTree(tree2OneSubtree);
- Tree<String> mergeTree = new Tree<>();
+ final Tree<String> mergeTree = new Tree<>();
mergeTree.addTree(tree1);
mergeTree.addTree(tree2);
- assertEquals(1, mergeTree.size());
- assertEquals(0, mergeTree.getObjectsAtDepth(0).size());
- assertEquals(1, mergeTree.getObjectsAtDepth(1).size());
- assertEquals(2, mergeTree.getObjectsAtDepth(2).size());
- assertEquals(3, mergeTree.getObjectsAtDepth(3).size());
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_1_1"));
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_2_1"));
- assertTrue(mergeTree.getObjectsAtDepth(3).contains("1_2_2"));
+ assertEquals(1, mergeTree.rootNodes().size());
+ assertEquals(1, mergeTree.getNodesAtDepth(0).size());
+ assertEquals(2, mergeTree.getNodesAtDepth(1).size());
+ assertEquals(3, mergeTree.getNodesAtDepth(2).size());
+ assertEquals(0, mergeTree.getNodesAtDepth(3).size());
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_1_1"));
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_2_1"));
+ assertTrue(mergeTree.getNodesAtDepth(2).contains("1_2_2"));
+ }
+
+ @Test
+ public void shouldGetOrCreateChild() {
+ final Tree<String> tree = new Tree<>();
+ final Tree<String> child = tree.getOrCreateChild("a");
+ assertNotNull(child);
+ assertTrue(child.isLeaf());
+ // calling again returns the same instance
+ assertSame(child, tree.getOrCreateChild("a"));
+
+ // mutating the returned subtree is observable through the parent
+ child.getOrCreateChild("a1");
+ assertTrue(tree.childAt("a").hasChild("a1"));
+ }
+
+ @Test
+ public void shouldAllowNullKeys() {
Review Comment:
You might want to rethink this. If you are going to implement this using maps in the other GLVs, null is not a valid key in a lot of languages.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#issuecomment-4695635826
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "GumpacG (via GitHub)" <gi...@apache.org>.
GumpacG commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3399673154
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/Tree.feature:
##########
@@ -206,36 +206,3 @@ Feature: Step - tree()
|--josh
|--peter
"""
-
- Scenario: g_V_out_treeXaX_selectXaX_countXlocalX
- Given the modern graph
- And the traversal of
- """
- g.V().out().tree("a").select("a").count(local)
Review Comment:
As per the [devlist post](https://lists.apache.org/thread/15lslfrs5f27hsz00k147mwc38o58w5k), it is not counting all nodes. Thanks for the suggestion.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#issuecomment-4626976107
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/3448?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 77.40%. Comparing base ([`a28cd1f`](https://app.codecov.io/gh/apache/tinkerpop/commit/a28cd1f2f1f10c140030a56de44c5c6c045af5f0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`969f6ce`](https://app.codecov.io/gh/apache/tinkerpop/commit/969f6ce0b3b3547beaf70414b13003f7d31e83fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 22 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3448 +/- ##
============================================
+ Coverage 76.35% 77.40% +1.04%
============================================
Files 1012 27 -985
Lines 60341 5980 -54361
Branches 7075 0 -7075
============================================
- Hits 46076 4629 -41447
+ Misses 11548 1074 -10474
+ Partials 2717 277 -2440
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/tinkerpop/pull/3448?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365761386
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Tree.java:
##########
@@ -20,125 +20,249 @@
import java.io.Serializable;
import java.util.ArrayList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
/**
+ * A tree data structure with a tree-shaped public API that is composed of a {@link HashMap}.
+ * <p>
+ * Internal storage is a plain {@link HashMap}, so iteration order is not promised and sibling-key collisions still
Review Comment:
To add a bit more to that, collapsing of sibling-key collisions is described as a bug or limitation when according to TINKERPOP-1693 it is expected/intended behaviour.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365528891
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Tree.java:
##########
@@ -20,125 +20,249 @@
import java.io.Serializable;
import java.util.ArrayList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
/**
+ * A tree data structure with a tree-shaped public API that is composed of a {@link HashMap}.
+ * <p>
+ * Internal storage is a plain {@link HashMap}, so iteration order is not promised and sibling-key collisions still
Review Comment:
Very small nit: I'm not sure you need to describe what the internals are. You just need to explain the limitations.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer merged PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make Tree no longer extend HashMap [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#issuecomment-4710983635
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org