Skip to content

feat(go,types): add FileType, Cardinality, AdjListType - #941

Open
ZekiLiu wants to merge 1 commit into
apache:mainfrom
ZekiLiu:828-go-sdk-types-enums
Open

feat(go,types): add FileType, Cardinality, AdjListType#941
ZekiLiu wants to merge 1 commit into
apache:mainfrom
ZekiLiu:828-go-sdk-types-enums

Conversation

@ZekiLiu

@ZekiLiu ZekiLiu commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Reason for this PR

Part of the pure-Go GraphAr SDK (tracking issus #828), following the bootstrap PR #937.
This is the foundational types package: the primitive value enums every higher
layer (info, reader/writer) depends on. Kept separate from DataType (next PR) so
each 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/types with four on-disk value types plus their string
parse/serialize and sentinel errors:

  • FileType — csv / parquet / orc / json
  • Cardinality — single / list / set
  • AdjListType — unordered/ordered × by_source/by_dest, with helpers to convert
    to/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 types package. No breaking changes (new code only).

Enum-style value types with parse/format reciprocity, Equal and the package
error sentinels. Table-driven unit tests.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.78%. Comparing base (54601c9) to head (ab8ea0e).

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              
Flag Coverage Δ
go 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@keksmd keksmd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants