[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 29 commits into
Conversation
ac8825a to
9d2e0fe
Compare
- LabelCardinality enum (ONE, ONE_OR_MORE, ZERO_OR_MORE) on Graph.Features
- Element.labels() returns Set<String>, addLabel()/dropLabels() mutation steps
- LabelsStep for g.V().labels() traversal
- LabelCardinalityValidator for constraint enforcement
- LabelsDropVerificationStrategy prevents accidental label removal
- MergeVertex supports multi-label via T.label list in merge map
- elementMap()/valueMap() label output controlled by with("multilabel")/with("singlelabel")
- GremlinLang propagates with("multilabel")/with("singlelabel") in gremlin text
- Grammar rules for addLabel(), dropLabels(), labels(), addV(String...)
- Java test infrastructure: World.getMultiLabelGraphTraversalSource(),
@MultiLabel/@MultiLabelDefault/@SingleLabelDefault tags
- GraphBinary V4 serialization for multi-label vertices/edges
- TinkerVertex stores labels as CopyOnWriteArraySet - TinkerGraph.vertexLabelCardinality configuration property - addLabel/dropLabels wired to TinkerVertex mutation - Unit tests for label cardinality, mutation, merge, and GremlinLang
- GremlinLang with("multilabel")/with("singlelabel") propagation in all GLVs
- Fix: render options in gremlin text both when OptionsStrategy is first created
and when it already exists (from prior with() calls like with("language",...))
- Go: With() method now variadic (single-arg g.With("multilabel") works)
- Feature tests: full matrix for valueMap/elementMap with @multilabel,
@MultiLabelDefault, @SingleLabelDefault tags
- Existing valueMap/elementMap tests use with("singlelabel") for provider safety
- Test infrastructure: @MultiLabelDefault uses gmultilabel + with("multilabel")
programmatically (no server-side YAML config needed)
- Docker gremlin-server-integration.yaml: multilabel graph config
- Cucumber support in all GLVs: terrain/world/steps handle multilabel graphs
- Upgrade docs: user guide for with("multilabel")/with("singlelabel"),
provider guide for LabelCardinality and step overrides
- Reference docs: elementMap/valueMap label format, TinkerGraph configuration
- Provider semantics: valueMap with-options section
- For-committers: @MultiLabel/@MultiLabelDefault/@SingleLabelDefault gherkin tags
- CHANGELOG entry
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?
| 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.
| 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?
|
|
||
| [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.
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
| * Upgrades the label set to a mutable ConcurrentHashMap-backed set on first mutation. | ||
| * This is a one-time cost per vertex that actually needs label mutation. | ||
| */ | ||
| private void ensureMutableLabels() { |
There was a problem hiding this comment.
TinkerGraph isn't built for concurrency i guess, but it might be good to update the javadoc to make it clear that we actively decided that concurrent label mutation on the same vertex is not safe.
| public long countVerticesByLabel(final String label) { | ||
| if (label == null) return getVerticesCount(); | ||
| return vertices.values().stream() | ||
| .filter(c -> c.get() != null && label.equals(c.get().label())) |
There was a problem hiding this comment.
Bad statistics to GQL planner if we don't account for multi-label.
| long count = 0; | ||
| final Iterator<Vertex> it = vertices(); | ||
| while (it.hasNext()) { | ||
| if (label.equals(it.next().label())) count++; |
There was a problem hiding this comment.
I sense we need to fix something here with label statistics for the GQL planner.
| this.vertexLabels.add(label); | ||
| Collections.addAll(this.vertexLabels, labels); | ||
| this.label = this.vertexLabels.iterator().next(); | ||
| this.graph.updateVertexLabelIndex(this); |
There was a problem hiding this comment.
we never update the vertexLabelCounts with this or with dropLabels below.
| edgelbl = r.to_object(b, DataType.list, False)[0] | ||
| # reading label list according to GraphBinaryV4 | ||
| edge_labels = r.to_object(b, DataType.list, False) | ||
| inv = Vertex(r.read_object(b), r.to_object(b, DataType.list, False)[0]) |
There was a problem hiding this comment.
is this reading just one label here? shouldn't it have logic like: https://github.com/apache/tinkerpop/pull/3483/changes#diff-7edc32e7bbb6b01ded55bbe1951532cb2807d565792ddc0ac1ba917bf497a844R66 ?
| if !ok || len(labelSlice) == 0 { | ||
| return nil, newError(err0404ReadNullTypeError) | ||
| } | ||
| label, ok := labelSlice[0].(string) |
There was a problem hiding this comment.
my Go isn't so smart, but possible problem accessing the index if there are no labels?
| .Primary components of the TinkerPop *structure* API | ||
| * `Graph`: maintains a set of vertices and edges, and access to database functions such as transactions. | ||
| * `Element`: maintains a collection of properties and a string label denoting the element type. | ||
| * `Element`: maintains a collection of properties and one or more string labels denoting the element type. |
There was a problem hiding this comment.
"one or more"? we have a zero or more config though. but maybe just re-write: "maintains a collection of properties with the option for multiple labels to help classify the element type."
| == Optional Data Model Variances | ||
|
|
||
| The simplest property graph model consists of vertices and edges, each with a single label and a set of key/value | ||
| properties. This is the lowest common denominator that every TinkerPop-enabled graph supports. Beyond that |
There was a problem hiding this comment.
Do we want to say that "every TinkerPop-enabled graph supports" this level? Seems like too strong a statement. Is it a "lowest common denominator" or the most common widely supported setup consistent with labelled property graph? I think there's some refinement needed here. good section though. Perhaps described as "advanced" modelling options?
| select('a','b').by('skill').by('name') // rank the users of gremlin by their skill level | ||
| ---- | ||
|
|
||
| [[vertex-labels]] |
There was a problem hiding this comment.
I think that since we make this introduction here, it would be good to describe use cases. This just tells the reader they can do it maybe but doesn't explain much about what it would be used for and when you should and shouldn't reach for it..
| per-traversal using `with("singlelabel")`. This is useful for providers that enable multi-label output by default | ||
| but need an escape hatch for backward compatibility. Note that when both options are present on the same source, | ||
| `"singlelabel"` always takes precedence regardless of the order in which they were applied. | ||
| * Serialization via GraphSON V1/V2/V3 or Gryo only transmits one label per vertex (using `label()`). Multi-label data |
There was a problem hiding this comment.
do we have a GraphSON v1/v2/v3 anymore in 4.0? We're running into Gryo issues now - is that single label note relevant once we fix that?
| serializer. As a result, Gryo-serialized `Tree` data written by 3.x cannot be read by 4.x and vice versa. The | ||
| Gryo type registration id (`61`) is unchanged and 4.x-to-4.x Gryo round-trips correctly; this Gryo break is | ||
| expected for the 4.0.0 major release. | ||
| ===== Multi-Label Support |
There was a problem hiding this comment.
Seems worth mentioning that the intent is that getLabelCardinality may be a static value if that's all the graph support but ultimately should reflect the state of the current graph instance. I'm not sure that's always clear to folks. There is a fair bit of detail here in the Upgrade docs for providers - possibly too much? Shouldn't we put these details in the Provider docs themselves and link to it from here with a more brief introduction?
| server-side label cardinality configuration. In that case, `with("singlelabel")` should still be respected as an | ||
| explicit user override back to single-string output. | ||
|
|
||
| Older serialization formats (GraphSON V1/V2/V3, Gryo) only support a single label and will silently use the value |
There was a problem hiding this comment.
More references to old serialization formats and the single label thing....
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
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