feat(go,types): add FileType, Cardinality, AdjListType - #941
Conversation
Enum-style value types with parse/format reciprocity, Equal and the package error sentinels. Table-driven unit tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
============================================
+ Coverage 77.40% 77.78% +0.37%
Complexity 615 615
============================================
Files 84 88 +4
Lines 8957 9109 +152
Branches 1069 1069
============================================
+ Hits 6933 7085 +152
Misses 1784 1784
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
keksmd
left a comment
There was a problem hiding this comment.
@ZekiLiu this is nicely built — the shared unexported name constants so String and Parse* cannot drift, the defensive slices.Clone on both the constructor and the getters, and the sentinel errors wrapped with %w are all the right instincts for a package this foundational. CI is green across Ubuntu Go 1.23, Ubuntu Go stable, golangci-lint and Pre-commit.
I compared the semantics against the C++ and Java implementations, since types is where a divergence would silently propagate into every reader built on top of it. Three findings, one of which I think is worth fixing before merge.
1. An unknown schema version is accepted and then fails silently
version2types only knows version 1, but nothing rejects other versions:
func (v InfoVersion) CheckType(typeStr string) bool {
if slices.Contains(version2types[v.version], typeStr) {For v.version == 2 the map lookup yields the nil slice, so CheckType returns false for bool, int32, int64, float, double and string alike. ParseInfoVersion("gar/v2") succeeds, Validate() passes, and the failure only surfaces much later as "every property type in this graph is unsupported".
Both other implementations refuse it at construction time. C++ (cpp/src/graphar/version_parser.h):
explicit InfoVersion(int version) : version_(version) {
if (version2types.find(version) == version2types.end()) {
throw std::invalid_argument("Unsupported version: " + std::to_string(version));
}
}so InfoVersion::Parse("gar/v2") returns Status::Invalid. maven-projects/info/.../VersionInfo.java carries the same table and the same expectation.
Suggest having ParseInfoVersion (and ideally Validate) reject a version that is not a key of version2types, so a future gar/v2 file fails loudly at the metadata boundary instead of degrading into a type-check failure.
2. The claimed C++ parity of ParseInfoVersion does not quite hold
The doc comment says:
The tolerances match the C++ parser so both read the same on-disk strings.
I found two inputs where they disagree. The C++ side uses std::regex_match (anchored, whole-string) with:
const std::regex version_regex("gar/v(\\d+).*");
const std::regex user_define_types_regex("gar/v\\d+ *\\((.*)\\).*");Leading whitespace. ParseInfoVersion starts with strings.TrimSpace(s), so " gar/v1" parses. regex_match(" gar/v1", version_regex) does not match, so C++ returns Status::Invalid. Java's VersionParser uses the identical pattern with Matcher.matches(), same result.
Position of the type list. The C++ pattern requires ( to follow the digits with only spaces in between. Go takes the first ( anywhere in the remainder:
if lparen := strings.IndexByte(rest, '('); lparen >= 0 {So "gar/v1 xyz(a,b)" yields userDefinedTypes = ["a","b"] in Go, and an empty type list in C++ and Java.
Neither is likely to appear in a real fixture, but since the comment promises parity it should either be made true or reworded to state where Go is deliberately more lenient. (For what it's worth "gar/v0" does agree — Go rejects it as non-positive, C++ rejects it as an unsupported version.)
3. Doc reference to code that does not exist yet
filetype.go:
// FileTypeJSON is the JSON text format. Parseable for read-only fixture
// compatibility but rejected by PropertyGroup.Validate.PropertyGroup is not part of this PR, so godoc renders a dangling reference. Either soften the wording now or land it together with the type it names.
Smaller note
NewInfoVersion silently clamps a non-positive version to DefaultVersion, while ParseInfoVersion returns an error for the same conceptual input. That asymmetry also makes the v.version <= 0 branch in Validate reachable only for the zero value. Returning an error from the constructor, or at least documenting the clamp as deliberate leniency for programmatic callers, would make the two paths easier to reason about.
None of this touches the AdjListType or Cardinality files, which read cleanly to me and match the on-disk spellings used by the other SDKs.
Reason for this PR
Part of the pure-Go GraphAr SDK (tracking issus #828), following the bootstrap PR #937.
This is the foundational
typespackage: the primitive value enums every higherlayer (info, reader/writer) depends on. Kept separate from
DataType(next PR) soeach PR stays a single, reviewable module. The diff is a bit over the usual size,
but about half is table-driven tests and the four enums are one cohesive,
dependency-free unit; splitting them further would fragment the value layer.
What changes are included in this PR?
New package
go/graphar/typeswith four on-disk value types plus their stringparse/serialize and sentinel errors:
FileType— csv / parquet / orc / jsonCardinality— single / list / setAdjListType— unordered/ordered × by_source/by_dest, with helpers to convertto/from the legacy
(ordered, aligned_by)form (src/dst)Enum members and on-disk spellings match the C++ and Java SDKs (verified
member-by-member); Rust and Python are FFI/bindings over C++. No Arrow or
third-party dependency.
Are these changes tested?
Yes. Table-driven unit tests cover round-trips, error paths, defensive copying,
and the lenient/strict version-string cases.
Are there any user-facing changes?
Yes — this adds the public
typespackage. No breaking changes (new code only).