[TINKERPOP-3261] Enable multiple labels on vertex with configurable label cardinality#3483
[TINKERPOP-3261] Enable multiple labels on vertex with configurable label cardinality#3483xiazcy wants to merge 40 commits into
Conversation
ac8825a to
9d2e0fe
Compare
9d2e0fe to
be5072e
Compare
| edges, properties) in the same order in which they were inserted into the graph. | ||
| * `@MetaProperties` - The scenario makes use of meta-properties. | ||
| * `@MultiLabel` - The scenario requires a graph that supports multi-label vertices (i.e. | ||
| `ZERO_OR_MORE` vertex label cardinality). Providers that only support single-label vertices should |
There was a problem hiding this comment.
is this truly ZERO_OR_MORE? like, have we structured it to be only that cardinality?
There was a problem hiding this comment.
Given the tests we currently have, I think this be a catch-all tag which essentially means "not single-label". I see this as similar to how we combined @multimetaproperties for many years before ultimately splitting them. I think it's ok to combine everything for now, but as we see what sort of configurations providers like to offer, we may want to split into more fine-grained control in the future. I could this evolve into a full set of tags including @MultiLabelV, @ZeroLabelV, @MultiLabelE, @ZeroLabelE.
| * `@MultiLabel` - The scenario requires a graph that supports multi-label vertices (i.e. | ||
| `ZERO_OR_MORE` vertex label cardinality). Providers that only support single-label vertices should | ||
| exclude these tests. | ||
| * `@MultiLabelDefault` - The scenario expects multi-label output as the default behavior for |
There was a problem hiding this comment.
is this correct? if it's testing a default, why are we setting with in this case. We didn't write that for single below. maybe the docs are just all off in this section?
There was a problem hiding this comment.
I agree the docs seem off here, the test infrastructure should not be explicitly setting with("multilabel") in these scenarios, they should be testing traversals which do not explicitly set with(), and the expectation is that the output would match a traversal with an with("multilabel") for these graphs.
There was a problem hiding this comment.
I've reworked the test infrastructure and updated these docs. @MultiLabelDefault and @SingleLabelDefault are for providers to opt-out of scenarios which assume the incorrect default behaviour for unconfigured traversals in their graph. Every provider should opt-out of one of these 2 tags.
One noteworthy caveat is that we never run any of the @MultiLabelDefault tests in TinkerGraph, as TinkerGraph does not have a configurable default here, it is always a SingleLabelDefault graph. I think this is acceptable, as there are relatively few tests using this tag, and I think we can just be careful when reviewing those scenarios to ensure they properly encode the expected semantics.
| * `moreLabels` - Additional labels to add. | ||
| * `labelTraversal` - A traversal that produces labels to add. | ||
|
|
||
| *Considerations:* |
There was a problem hiding this comment.
The graph must be configured with a
LabelCardinalitythat supports mutation (ONE_OR_MOREorZERO_OR_MORE).
"supports mutation"? First, that sounds a bit odd...LabelCardinality is not really "mutation" right? Does single imply immutability? is that right?
There was a problem hiding this comment.
I've reworded this for clarity, but yes singlelabel does imply immutability, because there is no atomic operation to replace labels, and there labels cannot be added or removed without violating the cardinality. This also keeps LabelCardinality.ONE consistent with the legacy TP3 semantics which I think is nice.
| ** 15 for including all | ||
|
|
||
| The label format in the map is controlled by source-level `with("multilabel")` and `with("singlelabel")` options, not | ||
| by the graph's `LabelCardinality`. When `"multilabel"` is present and `"singlelabel"` is not, the label value is a |
There was a problem hiding this comment.
When
"multilabel"is present and"singlelabel"is not,
that's an odd way to say that, no? it's not like they make any sense both being configured. ah, but then:
When both options are present on the same source,
"singlelabel"always takes precedence regardless of the order in which they were applied.
I'm not sure I like that. I suppose it's better than an error? Was this an explicit design choice?
There was a problem hiding this comment.
This has been rewritten to express the proper semantics. There is no more wording about precedence, as it's impossible to set both with options at the same time (rejected by StandardVerificationStrategy).
| g.V().properties() | ||
| ---- | ||
|
|
||
| [[tinkergraph-multi-label]] |
There was a problem hiding this comment.
We're missing a lot of docs updates. TinkerGraph isn't the story here. The story is that TinkerPop now has multi-label support. The entire reference guide, recipes, tutorials, etc. need review. At minimum, we have to conceptually introduce this change to the "structure" at the front end of the reference docs: https://tinkerpop.apache.org/docs/current/reference/#graph-structure We should be looking to explain this feature throughout the docs in a seamless fashion.
There was a problem hiding this comment.
I've made an attempt at these updates, I've added more multilabel context throughout the docs. I'm generally happy with the current state for the sake of this PR, however I also intend to do a thorough reading of the docs leading up to the next release and I suspect there may be more multi-label edits made at that time.
| has label "a" OR label "b". To match vertices having both labels, chain multiple `hasLabel()` calls: | ||
| `hasLabel("a").hasLabel("b")`. | ||
|
|
||
| IMPORTANT: Edge labels are currently fixed at cardinality `ONE` and are not configurable. |
There was a problem hiding this comment.
do we still even have a notion of edge cardinality?
There was a problem hiding this comment.
Not really. It was intended to act as an extra reminder for clarity, but now I think it's just unnecessary clutter. I've removed it.
|
|
||
| [gremlin-groovy] | ||
| ---- | ||
| conf = new BaseConfiguration() |
There was a problem hiding this comment.
as a follow-on task, should we perhaps add a JIRA to create a Builder of sorts for TinkerGraph configurations?
There was a problem hiding this comment.
| [[droplabel-step]] | ||
| === DropLabel Step | ||
|
|
||
| The `dropLabel()`-step (*sideEffect*) removes one or more specific labels from an element. The `dropLabels()`-step |
There was a problem hiding this comment.
I really dislike that our reference documentation is reading like semantics. For example:
Dropping a label that does not exist on the element is a no-op.
Leave specification statements for semantics. if we want to talk about a no-op condition then we should introduce a scenario that sets up that case. That statement on its own does not naturally flow from the prior sentence. The tinker-doc skill continues to need refinement, but even then, I think we need to pay much closer attention to what is being produced here. Perhaps we've already let too much of this shortened language into the reference docs and it all needs review.
| * Adds dynamically computed labels to the current element. This is a side-effect step that passes the | ||
| * element through unchanged. | ||
| * | ||
| * @param labelTraversal the traversal that produces labels to add |
There was a problem hiding this comment.
the traversal that produces labels to add
labels plural? I assume it's not the case that we are going to iterate out all the Strings this traversal produces right? it's just pulling the first one, no? if not, i think that's an issue because that would cut against the by rule. i'm not sure what that means though because then we have nothing analogous to addLabel(final String label, final String... moreLabels) - not quite as nice to do multiple addLabel(Traversal) which may get ugly if you're trying to pick apart a list of labels. i dunno...not sure what we want to do here.
There was a problem hiding this comment.
This has been updated to follow the standard traversal varargs pattern. All traversals get iterated once, if there is exactly one traversal provided and it produces a Collection, that collection gets automatically spread into the label set.
| e.setId(deserializationContext.readValue(jsonParser, Object.class)); | ||
| } else if (jsonParser.getCurrentName().equals(GraphSONTokens.LABEL)) { | ||
| jsonParser.nextToken(); | ||
| final java.util.Set<String> labels = new java.util.LinkedHashSet<>(); |
There was a problem hiding this comment.
necessary for edges? they don't have multilabel concepts right?
There was a problem hiding this comment.
Following the existing IO docs in master, the edge label is already encoded as a list in the wire format. I'd like the serializers to operate on edge labels as lists and leave the EdgeCardinality=1 enforcement to the structure API. I think that gives us the most flexibility for future changes, even though there are currently no plans to have multilabelled edges.
| final Object id = context.read(buffer); | ||
| // reading single string value for now according to GraphBinaryV4 | ||
| final String label = (String) context.readValue(buffer, List.class, false).get(0); | ||
| // Read all labels as List<String> for multi-label support |
There was a problem hiding this comment.
no multi-label for edge, right?
There was a problem hiding this comment.
I don't think we should support multi-label or zero-label edges right now, but I think we should align the structure API and serializers to present the single label in a list anyways, it gives symmetry with Vertex as children of Element, and it sets us up for a future data model extension without the same hard breaks.
|
What are the implications for GraphML? Has that been considered? At minimum, there are some documentation concerns to take into account. Maybe, there's a knob on the IO builder to do the colon separator thingy. At least then it would still work. maybe other knobs to help it work in different capacities? if you don't want to figure it out for this PR, that's fine, create a JIRA that blocks for official 4.x and add appropriate warning docs to explain the current behavior ( i assume random label). |
| edges, properties) in the same order in which they were inserted into the graph. | ||
| * `@MetaProperties` - The scenario makes use of meta-properties. | ||
| * `@MultiLabel` - The scenario requires a graph that supports multi-label vertices (i.e. | ||
| `ZERO_OR_MORE` vertex label cardinality). Providers that only support single-label vertices should |
There was a problem hiding this comment.
Given the tests we currently have, I think this be a catch-all tag which essentially means "not single-label". I see this as similar to how we combined @multimetaproperties for many years before ultimately splitting them. I think it's ok to combine everything for now, but as we see what sort of configurations providers like to offer, we may want to split into more fine-grained control in the future. I could this evolve into a full set of tags including @MultiLabelV, @ZeroLabelV, @MultiLabelE, @ZeroLabelE.
| * `@MultiLabel` - The scenario requires a graph that supports multi-label vertices (i.e. | ||
| `ZERO_OR_MORE` vertex label cardinality). Providers that only support single-label vertices should | ||
| exclude these tests. | ||
| * `@MultiLabelDefault` - The scenario expects multi-label output as the default behavior for |
There was a problem hiding this comment.
I agree the docs seem off here, the test infrastructure should not be explicitly setting with("multilabel") in these scenarios, they should be testing traversals which do not explicitly set with(), and the expectation is that the output would match a traversal with an with("multilabel") for these graphs.
| final Object id = context.read(buffer); | ||
| // reading single string value for now according to GraphBinaryV4 | ||
| final String label = (String) context.readValue(buffer, List.class, false).get(0); | ||
| // Read all labels as List<String> for multi-label support |
There was a problem hiding this comment.
I don't think we should support multi-label or zero-label edges right now, but I think we should align the structure API and serializers to present the single label in a list anyways, it gives symmetry with Vertex as children of Element, and it sets us up for a future data model extension without the same hard breaks.
The single-label-default valueMap/elementMap scenarios were previously weakened to a single-label vertex, defeating their purpose. Restore the multi-label vertex and assert the (non-deterministic) single-label result is one of the possible label renderings using 'the result should be of'. Assisted-by: Kiro:claude-opus-4.8
Add addLabel(Traversal, Traversal...) and dropLabel(Traversal, Traversal...) overloads so both steps accept one-or-more label-producing traversals, matching the pattern already used by addV(Traversal, Traversal...): a single traversal that produces a Collection<String> is auto-unfolded, while multiple traversals must each resolve to a scalar String. - AddLabelStep and DropLabelsStep now share a single List<Traversal.Admin> field instead of separate single/multi fields, resolved via the new TraversalUtil.resolveStringArguments() helper. DropLabelsStep gains Collection-unfold support for the single-traversal case, which it previously lacked (inconsistent with AddLabelStep). - Add TraversalHelper.asVarargsList() to share first+more null-checking and list-building across GraphTraversal's varargs spawn methods. - Update Gremlin.g4 and TraversalMethodVisitor to parse comma-separated nestedTraversal lists for addLabel()/dropLabel(), mirroring addV(). - Add matching ITraversal-vararg overloads to Gremlin.Net (Python, JS, and Go already accept variadic traversal arguments). - Add grammar visitor unit tests and Gherkin scenarios covering single-traversal collection unfold, multi-traversal varargs, and the multi-traversal-collection error case for both steps; regenerate translations.json from the new scenarios. - Update gremlin-semantics.asciidoc, the-traversal.asciidoc, and CHANGELOG.asciidoc for the new syntax. Assisted-by: Kiro:claude-sonnet-5
Introduce a one-or-more stringArgumentVarargs grammar rule (distinct from the existing zero-or-more stringNullableArgumentVarargs) and use it for addV, addLabel, and dropLabel's string forms, replacing manual first+more splitting loops in TraversalMethodVisitor and TraversalSourceSpawnMethodVisitor with a consistent up-front variable-check followed by a single parseString/parseStringVarargs call. - Add ArgumentVisitor.parseString(StringArgumentContext) and parseStringVarargs(StringArgumentVarargsContext), paralleling the existing StringNullableArgumentContext overloads, and use them to simplify addE's string-argument visitors as well. - Label the previously-unlabeled alternatives of traversalSourceSpawnMethod_addV (Empty/String/Traversal) so TraversalSourceSpawnMethodVisitor dispatches to three focused methods instead of one method that manually disambiguates via null/isEmpty checks, matching the pattern already used by traversalMethod_addV. - Update DefaultGremlinBaseVisitor and DotNetTranslateVisitor for the newly labeled alternatives and the new grammar shape; DotNetTranslateVisitor now walks into the nested stringArgumentVarargs child instead of assuming a flat StringArgumentContext child, which the prior grammar shape allowed. No behavioral changes intended; verified via the full gremlin-core unit test suite and the addLabel/dropLabel/multi-label Cucumber scenarios. Assisted-by: Kiro:claude-sonnet-5
The generated addV(String, String...) override referenced an undefined 'first' variable after a parameter rename, and neither generated addV overload actually threaded additionalLabels through to AddVertexStartStep, silently dropping labels beyond the first when called through a custom Gremlin DSL. - Mirror GraphTraversalSource.addV's branching logic in the generated addV(String, String...) and addV(Traversal, Traversal...) methods so additionalLabels are aggregated into a Set/List and passed to AddVertexStartStep. - Add a runtime test (AddVMultiLabelDslTraversal fixture) asserting that all labels are applied, not just compilation success. - Touch up addV/addLabel/dropLabel javadocs in GraphTraversal and replace a java.util wildcard import with explicit imports in GraphTraversalSource. - Regenerate GLV feature test step definitions. Assisted-by: Kiro:claude-sonnet-5
addLabel(String, String...) and dropLabel(String, String...) have no GValue overload, but their grammar used stringArgumentVarargs, which supports parameter-bound variables. TraversalMethodVisitor parsed those arguments as GValue<String> and then immediately called .get() on each one to satisfy the plain-String signature, silently discarding any variable binding a user supplied. - Add a one-or-more stringLiteralVarargs grammar rule (literal-only, no variable alternative) distinct from the GValue-aware stringArgumentVarargs, and use it for addLabel/dropLabel's string forms. - Add GenericLiteralVisitor.parseStringVarargs(StringLiteralVarargsContext) returning a plain String[], paralleling the existing StringNullableLiteralVarargsContext overload, and use it in TraversalMethodVisitor in place of the GValue parse-then-unwrap. - Add the corresponding DefaultGremlinBaseVisitor override for the new grammar rule. Verified via gremlin-core (mvn verify, 9144 tests) and gremlin-language (mvn test, 3920 tests). Assisted-by: Kiro:claude-sonnet-5
Collapse the separate Set<Object>/List<Traversal.Admin<?,?>> label representations into a single Collection<Object>-typed label field across AbstractAddElementStepPlaceholder, AbstractAddVertexStepPlaceholder, AddVertexStepPlaceholder, and AddVertexStartStepPlaceholder, with a private canonical constructor and setLabel() as the single source of truth for label normalization (constructor and post-construction calls now behave identically). Fix AddVertexStep/AddVertexStartStep: introduce a single 'label' field replacing the disconnected internalParameters T.label entry and the separate labelTraversals field, so getLabel()/setLabel() are no longer blind to multi-traversal labels. getLabel() resolves ConstantTraversal/GValueConstantTraversal elements to their String value where possible, and otherwise returns the unresolved Traversal itself (consistent with existing AddEdgeStep behavior), rather than silently dropping or throwing on unresolvable elements. Add/update javadoc on AddElementStepContract.getLabel()/setLabel() to document actual return/accepted types and drop the previously-implied 'set once' contract, which is not honored uniformly by all implementations (e.g. AddEdgeStep permits overwriting). Assisted-by: Kiro:claude-sonnet-5
- ComputerElement.labels() previously inherited Element's default
implementation (Collections.singleton(label())) instead of delegating
to the wrapped element, silently losing multi-label data and returning
a nondeterministic single label under GraphComputer/OLAP execution.
- ElementMapStep.getVertexStructure() always rendered adjacent vertex
labels via the deprecated, nondeterministic label() regardless of
with("multilabel") mode, ignoring the same multilabel option that the
top-level element already respected. Corrected two scenarios in
ElementMap.feature whose expected values assumed the old behavior.
- gremlin-javascript's DotNetTranslateVisitor never unwrapped the
StringArgumentVarargsContext node introduced for multi-label addV(),
so generated .NET translations were missing the (string) cast that
the Java translator already applies; also fixed a labeled-alternative
method name mismatch that left visitTraversalSourceSpawnMethod_addV
as dead code (never dispatched by the ANTLR visitor).
Assisted-by: Claude Code:claude-sonnet-5
…er relocation - StandardVerificationStrategy: use getStepsOfAssignableClass and also reject label().drop() in addition to labels().drop() - StandardVerificationStrategyTest: add label().drop() rejection cases - EventUtil.registerLabelChange: drop unnecessary unchecked cast/suppression - Relocate WithOptions.isMultilabelEnabled to TraversalHelper.isMultilabelEnabled as it fits traversal-inspection utilities better than a with()-options constants class; update ElementMapStep/PropertyMapStep callers - AddVertex.feature: use constant() instead of inject().fold() in the addV multi-label error scenario for clarity; propagate the scenario rename/traversal change to translations.json and the GLV cucumber mapping files (.NET, Go, JavaScript, Python) Assisted-by: Kiro:claude-sonnet-5
The @MultiLabelDefault/@SingleLabelDefault tags describe a provider's
unconfigured (no explicit with("multilabel")/with("singlelabel")) label
output format for valueMap()/elementMap() and are mutually exclusive per
provider - a provider must exclude whichever one it does not natively
implement. This applies independently of @multilabel (multi-label
mutation capability).
Previously every World implementation (Java, and the equivalent glue in
.NET/Go/JS/Python) faked @MultiLabelDefault by injecting
with("multilabel") onto the traversal source, which does not actually
exercise default (unconfigured) behavior and gives false confidence.
None of the current providers (TinkerGraph, remote gremlin-server)
implement genuine multilabel-by-default semantics, so:
- Removed World.getMultiLabelDefaultGraphTraversalSource() and its
RemoteWorld override, and the corresponding fakes in the Go/JS/Python
glue code and .NET CommonSteps - none had any purpose beyond this
faking.
- Added "not @MultiLabelDefault" (or the equivalent skip/ignore
mechanism) to every existing feature test runner across Java
(@CucumberOptions tags, including the shared GRAPHCOMPUTER_TAG_FILTER),
.NET (GherkinTestRunner ignore list), Go (Tags filter string and
ErrPending skip), JavaScript (Before hook skip), and Python (ignore
tagset check).
Also addressed a coverage gap: every existing scenario tagged with
@SingleLabelDefault/@MultiLabelDefault was also tagged @multilabel, but
the default label-format question applies independently of multi-label
mutation capability - a single-label-only graph still has to decide
whether T.label in valueMap()/elementMap() is a String or a singleton
Set<String> by default. Added new scenarios without @multilabel on the
plain modern graph exercising both the @SingleLabelDefault and
@MultiLabelDefault cases for elementMap() and valueMap().with(tokens),
propagated to translations.json and the GLV cucumber mapping files.
Assisted-by: Kiro:claude-sonnet-5
- ValueMap.feature / ElementMap.feature: fix the @MultiLabelDefault
single-label-vertex scenarios that assumed the arbitrary label
returned for a multi-labeled vertex would always be "person" -
switched to "the result should be of" with both possible label
options, matching the pattern already used for the analogous
@SingleLabelDefault scenarios.
- AddLabel.feature: add a multi-label-argument error scenario
(addLabel("a", "b")) on a single-label graph, alongside the existing
single-label-argument case.
- Split the deprecated label() step scenarios out of Labels.feature
into a new Label.feature, matching the AddLabel.feature/
DropLabel.feature naming convention for the deprecated singular step
vs. the labels() plural step. Added coverage for label() on a
single-label graph and on edges in a multi-label graph, which were
previously untested.
- MergeVertex.feature: add single-label-graph error scenarios for
mergeV() attempting to create/match with multiple labels via the
merge map, option(Merge.onCreate, ...), and option(Merge.onMatch, ...).
Propagated all scenario additions/changes to translations.json and the
GLV cucumber mapping files (.NET, Go, JavaScript, Python).
Assisted-by: Kiro:claude-sonnet-5
TinkerWorld's shared empty-graph configuration forced ZERO_OR_MORE vertex label cardinality unconditionally, meaning "the empty graph" and the @MultiLabel-routed multi-label graph were the same underlying config for TinkerGraph. This made it impossible to write a single-label-graph error scenario using the empty graph, since mutation there always succeeded under multi-label semantics instead of failing as intended. Split the fixture to mirror how RemoteWorld already separates its plain `g` traversal source (single-label) from `gmultilabel` (multi-label): - getNumberIdManagerConfiguration() no longer forces ZERO_OR_MORE, restoring TinkerGraph's real default of ONE for the plain empty graph. - Added getMultiLabelConfiguration(), explicitly ZERO_OR_MORE, and wired getMultiLabelGraphTraversalSource() overrides for TinkerGraphWorld, TinkerTransactionGraphWorld, and TinkerShuffleGraphWorld so @multilabel scenarios keep working against their own dedicated multi-label graph. - Removed TinkerTransactionGraphWorld's beforeEachScenario hack that previously skipped single-label-graph error scenarios outright because the old shared config could never produce a single-label graph. Reworked the DropLabel.feature, AddLabel.feature, and MergeVertex.feature single-label-graph error scenarios (added in this PR) to use the empty graph with a graph initializer instead of the modern graph, since only the empty graph is meant to be mutated by scenarios. Also removed @GraphComputerVerificationStrategyNotSupported from these scenarios - it turned out to be unnecessary since ComputerWorld already skips any scenario using the empty graph via an AssumptionViolatedException, regardless of tags. Propagated the scenario traversal changes to translations.json and the GLV cucumber mapping files (.NET, Go, JavaScript, Python). Assisted-by: Kiro:claude-sonnet-5
…semantics
with("multilabel") and with("singlelabel") are mutually exclusive. Rather
than silently resolving the conflict via precedence, StandardVerificationStrategy
now rejects traversal sources that configure both, throwing VerificationException
during strategy application.
- StandardVerificationStrategy: reject sources with both options present
- TraversalHelper.isMultilabelEnabled: simplified now that the conflicting
state is rejected upstream
- WithOptions: javadoc updated to describe mutual exclusivity
- gremlin-semantics.asciidoc: replaced precedence-rule prose with the new
rejection behavior; added missing elementMap() semantics entry
- for-committers.asciidoc: minor wording clarifications for @MultiLabel/
@MultiLabelDefault/@SingleLabelDefault tag docs
- GLV cucumber/feature files: reorder relocated test entries
Assisted-by: Kiro:claude-sonnet-5
- gremlin-semantics.asciidoc: remove the misplaced hasLabel() OR-semantics
note from the labels() section
- the-traversal.asciidoc: add a live hasLabel('person','software') example
with callout demonstrating OR semantics in the Has Step reference section;
fix a pre-existing has()/hasLabel() argument mismatch in the example list
- the-traversal.asciidoc: reword 'supports mutation' to 'permits changing
the set of labels on an element' in AddLabel/DropLabel step docs, matching
the wording already used in gremlin-semantics.asciidoc
Assisted-by: Kiro:claude-sonnet-5
Addresses PR review Thread 6: introduce multi-label vertices as a natural part of the docs rather than confined to TinkerGraph-specific content. - the-graph.asciidoc: new 'Optional Data Model Variances' section frames multi-properties, meta-properties, multi-label, and nullable properties as a unified category of provider-optional data model features, discoverable via Feature. Vertex Labels and Vertex Properties become sibling subsections; adds a new Nullable Property Values subsection (previously undocumented in the reference guide). - intro.asciidoc: minimal touch - fixes the inaccurate 'a string label' wording and adds a single pointer to the new section, without loading foundational structure content with cardinality nuance. - the-traversal.asciidoc: notes that by(label)/T.label (group(), groupCount(), dedup(), order(), path(), simplePath(), tree(), choose()) are non-deterministic for multi-label vertices; new mergeV() subsection documenting AND-semantics for label search and addLabel-style append semantics for onCreate/onMatch, verified against runtime behavior. - New recipes/multi-label-vertices.asciidoc recipe covering addLabel/ dropLabel, hasLabel OR-semantics vs AND-chaining, label() vs labels(), grouping pitfalls, and mergeV(); every example verified against actual TinkerGraph output before inclusion. - anti-patterns.asciidoc: notes that the by(label) token pattern it recommends breaks down for multi-label vertices, linking to the new recipe. - getting-started tutorial: softens singular-label framing, adds brief forward pointers without disrupting introductory flow. Assisted-by: Kiro:claude-sonnet-5
…t recipe Addresses the broader concern in PR review Thread 11: reference documentation should read as illustrated, flowing prose rather than terse specification statements. - the-traversal.asciidoc: AddLabel/DropLabel/Label/By/Has/MergeVertex step sections rewritten so behavioral rules (no-ops, exceptions, non-determinism, AND/OR semantics) are demonstrated with a scenario rather than asserted in isolation. Fixed stale 'singlelabel always takes precedence' language left over from the earlier VerificationException fix (Thread 5). - implementations-tinkergraph.asciidoc: wove the edge-single-label constraint into surrounding prose instead of a standalone IMPORTANT callout. - Removed docs/src/recipes/multi-label-vertices.asciidoc: on review, its content was a feature explanation, not a domain-agnostic traversal pattern, and everything in it is now covered directly in the-traversal.asciidoc (AddLabel/DropLabel/Has/Label/Labels/By/MergeVertex step sections). Re-pointed the two cross-references that linked to it (anti-patterns.asciidoc, getting-started tutorial) at the reference guide's By Step section instead. All executable examples were run against real TinkerGraph output before being included. Assisted-by: Kiro:claude-sonnet-5
StarGraphGraphSONSerializerV4/StarGraphGraphSONDeserializer only ever wrote/read a vertex's label as a single string, even under GraphSON V4, so a multi-label vertex silently lost all but one of its labels when serialized through the StarGraph codec. Fixes the V4 serializer to write the full label set as an array (matching the convention already used by the element-level V4 serializers for the driver protocol) and the shared deserializer to read either a single string (V1/V2/V3) or an array (V4) back into a StarVertex's label set. Assisted-by: Kiro:claude-sonnet-5
StarGraphSerializer's Kryo format only ever wrote/read a vertex's label as a single string, so a multi-label vertex silently lost all but one label when serialized. Introduces a VERSION_2 format byte that writes the full label set as a list; VERSION_1 (single label) data written by prior versions of this serializer remains readable via a version-branched reader, so existing .kryo fixtures are unaffected. Attachable.Method.createVertex (used by GryoReader.readGraph and other providers' attach-to-host-graph logic) had the same gap - it read a multi-label source vertex correctly but only ever passed a single label to the new vertex. Fixed to call addLabel() for any labels beyond the first. Also fixes HadoopElement, which delegated label() to its wrapped element but never overrode labels(), silently falling back to Element's single-label default (Collections.singleton(label())) regardless of how many labels the underlying vertex actually had. Assisted-by: Kiro:claude-sonnet-5
Rework the multi-label representation so edges and vertex-properties stay single-label and only vertices carry multi-label state: - DetachedElement/ReferenceElement: single String label with no dual-state branching. The multi-label set now lives only in DetachedVertex/ ReferenceVertex and is left null for the common single-label case, which preserves the Kryo field-serialized form (avoids shifting Gryo reference ids in the provider-coercion path). - StarVertex: vertexLabels is the sole source of truth; label() and the GraphSON serializers read via the method rather than the raw label field. - TinkerVertex: drop the redundant this.label sync; vertexLabels is the source of truth. Tests (integrated into existing classes): - VertexWritableTest: multi-label round-trip through the Hadoop OLAP carrier (VertexWritable -> StarGraph via KryoShim). - SparkIoRegistryCheck / AbstractIoRegistryCheck: end-to-end SparkGraph multi-label round-trip using GraphSON v4 file IO (GraphSONInputFormat + SparkGraphComputer), asserting all labels survive on star-center results. Assisted-by: Kiro:claude-opus-4.8
TinkerVertex.addLabel/dropLabel/dropLabels invoked the updateVertexLabelIndex hook but TinkerGraph never overrode it, so vertexLabelCounts only tracked labels set at vertex creation and went stale after label mutations. The hook now passes the pre-mutation label set so TinkerGraph can diff and adjust counts precisely. AbstractTinkerGraph's default countVerticesByLabel/countEdgesByLabel, and TinkerTransactionGraph's override, filtered on the deprecated single-value label() accessor, undercounting vertices/edges with labels beyond the first. Both now check labels().contains(label). Addresses unresolved review comments on PR #3483 regarding stale GQL planner cardinality statistics for multi-label vertices. Assisted-by: Kiro:claude-sonnet-5
…-safety Replace the diff-based updateVertexLabelIndex hook, which took the vertex and its previous label set, with two delta-based hooks, addVertexLabels/removeVertexLabels, that receive only the labels actually added or removed. TinkerVertex's addLabel/dropLabel/dropLabels already know these deltas at the call site, so this avoids copying and diffing the full label set on every mutation in favor of O(k) work for the k labels being changed. Also documents on TinkerVertex.vertexLabels and ensureMutableLabels that concurrent label mutation on the same vertex across threads is not supported, consistent with TinkerGraph's general concurrency model. Addresses review feedback on PR #3483. Assisted-by: Kiro:claude-sonnet-5
EdgeIO._read_edge read the full label list for the edge itself but only kept the first label (index 0) for the inV/outV endpoint vertices, discarding the rest and never passing a labels= list to Vertex(). This was asymmetric with EdgeIO.dictify, which already serializes the full label set for both endpoints, and with VertexIO._read_vertex, which correctly preserves all labels. Endpoint vertices for a multi-labelled Vertex now round-trip with their full label set intact when read back via a deserialized Edge. Addresses review feedback on PR #3483 (thread 16). Assisted-by: Kiro:claude-sonnet-5
Python: - EdgeIO._read_edge no longer drops endpoint (inV/outV) labels: it read the full label list for the edge but only kept the first label for its endpoint vertices, discarding the rest. Both endpoints now preserve their complete label set, matching EdgeIO.dictify and _read_vertex. - Vertex and Edge constructors now treat an explicitly provided but empty labels argument as a genuine zero-label element, rather than collapsing it to the default label. The deprecated singular label returns "" for a label-less element, consistent with the Java and .NET drivers. - VertexIO/EdgeIO.dictify now serialize a present-but-empty label set as an empty list instead of falling back to [label], so zero-label elements round-trip correctly. .NET: - GraphSerializer wrote the singular Label wrapped in a one-element list when serializing vertices/edges inside a Graph, producing [""] for a zero-label vertex instead of []. It now writes the full Labels set, matching the dedicated VertexSerializer/EdgeSerializer. Adds Python round-trip tests for multi-label and zero-label vertices and edge endpoints. Addresses review feedback on PR #3483 (thread 16). Assisted-by: Kiro:claude-sonnet-5
The Go driver already handled multi-label vertices, but zero-label vertices were rejected: readVertex/readEdge errored on an empty label list (reusing the misleading E0404 null-type error), and the writers fell back to the singular Label whenever Labels was empty, so a zero-label element could never be produced or consumed. - readVertex/readEdge now accept an empty label list, collecting the full label set and setting the deprecated singular Label to the first label, or "" when there are none (consistent with Java, .NET, Python). - vertexWriter/edgeWriter (and the edge endpoint writers) now trigger the legacy single-Label fallback only when Labels is nil (never populated), treating a non-nil empty slice as an authoritative zero-label element. - Removed the getVertices workaround in the Cucumber world that projected id/label as primitives to avoid deserializing zero-label vertices. The feature tests now deserialize real Vertex objects, so they genuinely exercise multi-label and zero-label handling instead of masking the gap. Adds a GraphBinary round-trip test for multi-label and zero-label vertices and edge endpoints. Relates to TINKERPOP-3261. Assisted-by: Kiro:claude-sonnet-5
The Graph-typed GraphBinary path (graphWriter/readGraph) is a separate inline implementation from vertexWriter/edgeWriter/readVertex, and it had no multi-label support at all: it wrote only the singular Label as a one-element list, read back only the first label, never populated Vertex.Labels/Edge.Labels, and rejected empty label lists. - graphWriter now writes the full vertex and edge label sets, falling back to the singular Label only when Labels is nil (zero-label elements with a non-nil empty slice serialize as an empty list). - readGraph now reads the full label list (allowing empty), populates Vertex.Labels and Edge.Labels, and sets the deprecated singular Label to the first label or "" when there are none. VertexProperty labels and the edge endpoint null placeholders are left as-is (single-value key and reference-only context respectively). Extends the GraphBinary round-trip test with a multi-label / zero-label graph. Relates to TINKERPOP-3261. Assisted-by: Kiro:claude-sonnet-5
Assisted-by: Kiro:claude-sonnet-5
Adds round-trip GraphBinary coverage for multi-labelled and zero-labelled
vertices, edges with multi/zero-labelled endpoints, and graphs containing
multi/zero-labelled vertices, integrated into each GLV's existing
GraphBinary test class. These cases previously had no coverage in any GLV,
which is how the multi-label/zero-label serialization bugs went unnoticed.
The new coverage exposed two further zero-label bugs, fixed here:
- Python TinkerGraphIO.dictify sent a zero-label vertex/edge down the
[label] fallback, serializing [""] instead of []. Now serializes the
full (possibly empty) label set, matching VertexIO/EdgeIO.
- .NET GraphSerializer read side (ReadVertexAsync/ReadEdgeAsync) extracted
only the first label and never passed the full set to the constructors,
so multi-label graph vertices lost labels and zero-label became {""}.
Now populates the full Labels, matching VertexSerializer/EdgeSerializer.
JS required no serializer change; its zero-label tests assert on the
labels Set only, since the singular label still returns "vertex".
Relates to TINKERPOP-3261.
Assisted-by: Kiro:claude-sonnet-5
…ip tests The Java GraphBinary GraphSerializer (the Graph-type serializer) only handled a single vertex/edge label: writeVertex wrote the singular label as a one-element list ([""] for zero-label), and readValue read only the first label. This mirrors the same graph-serializer gap fixed in the Go, .NET and Python GLVs. - GraphSerializer.writeVertex now writes the full vertex.labels() set (empty list for zero-label). - GraphSerializer.readValue parses vertices into DetachedVertex instances first, so the required vertex label cardinality can be determined from the data, then opens the graph with ZERO_OR_MORE only when a multi- or zero-label vertex is actually present (single-label graphs keep the default ONE). Vertices are materialized via Attachable.getOrCreate. - Attachable.createVertex mishandled zero-label vertices: it called addVertex(T.id, id, T.label, label()) where label() is "" for a label-less vertex, producing a [""]-labelled vertex. It now adds the vertex without T.label when labels() is empty. The multi-label path is unchanged. Adds label-aware round-trip coverage in AbstractRoundTripTest for multi-label and zero-label vertices, edges with multi/zero-label endpoints, and graphs with multi/zero-label vertices. These run through both GraphBinary and GraphSON V4; the two graph cases are GraphBinary-only because GraphSON V4's message serializer does not support Graph-type objects (pre-existing limitation). Relates to TINKERPOP-3261. Assisted-by: Kiro:claude-sonnet-5
Soften the "every TinkerPop-enabled graph supports" claim and frame the optional data-model variances as advanced options. Weave multi-label use cases and when-to/when-not-to guidance into the Vertex Labels section. Clarify getLabelCardinality() instance semantics in the provider upgrade notes and remove a cross-reference to a provider-docs anchor that does not exist yet. Assisted-by: Kiro:claude-opus-4.8
7042a03 to
165ab5b
Compare
Introduces configurable label cardinality for graph elements. Vertex cardinality is user-configurable; edge cardinality defaults to ONE and is not yet exposed as a configuration option but uses the same underlying infrastructure.
Vertices can now have zero, one, or many labels controlled by LabelCardinality (defaults to ONE for full backward compatibility). New traversal steps labels(), addLabel(), dropLabel(), and dropLabels() enable label retrieval and mutation. Edge labels remain fixed at cardinality ONE.
Notes on design:
Commits:
Configuration
To Enable multi-label in TinkerGraph:
gremlin.tinkergraph.vertexLabelCardinality=ZERO_OR_MORE
Testing
VOTE +1