Revive repo: CI, Tests and type system (partial) (closes #3)

[anasik]

Summary

This repo has had no real activity since 2022. I'm reviving it incrementally — this PR now covers three things.

Build/repo hygiene

• Bump substrait submodule v0.42.1 → v0.99.0; retarget Substrait.Core to .NET 10 (LTS)
• Add .editorconfig, .gitattributes, Directory.Build.props (real code-style analysis in the build, not just whitespace), global.json
• Migrate to .slnx; remove dead x64/x86 platform entries
• CI: format-check/build/test on push+PR, hardened (least-privilege permissions, concurrency cancellation, SHA-pinned actions, workflow_dispatch)
• Migrate test project to xunit v3
• Flip CompileOutputs to true — the generated protobuf code is now actually compiled and build-verified, not just generated and ignored
• Fix SubstraitRelVisitor: it was abstract with zero virtual members, so every subclass threw on every call — the visitor pattern was non-functional. Now overridable.
• Fix namespace/folder mismatches across Relation/*

Closes #3.

Core type system

• TypeClass + ITypeVisitor (with a VisitFallback default so adding a kind isn't a breaking change) + TypeCreator, covering every Simple/Compound kind except UserDefined/NSTRUCT (deliberately
  deferred, see comment below)
• Namespace renamed Substrait.Core.Type → Substrait.Core.Types to avoid the System.Type shadowing problem raised in review
• TypeCreator validates constructor arguments against the spec's bounds
• Fix a defensive-copy bug in Struct/Func: both stored the caller's array reference directly, so mutating it after construction silently changed the record's hash and could corrupt a dictionary/set it
  was keyed into
• Add test coverage for the type system: equality/hashing edge cases (including a regression test proving the defensive-copy fix above), and a reflection-based check that every TypeClass has a matching
  ITypeVisitor overload

(Correcting the original text here, which overstated this as "every Simple/Compound type kind.")

Still open

• UserDefined/NSTRUCT — deliberately deferred, see comment below for why

[CLAassistant]

All committers have signed the CLA.

[nielspardon]

Thanks for picking this up — the repo genuinely needed reviving, and modelling the type system on substrait-java's TypeCreator/Type split is the right call.

Disclosure: C# isn't my main language, and I used an AI assistant to work through this diff. I've tried to keep the two kinds of claim separate below — the compile-behaviour ones come with repros you can check in under a minute, and the API-shape ones are opinions to weigh against your own plans for the library. None of it is build-verified, since CI hasn't run on the PR yet. Corrections welcome, particularly on the spec bounds in the TypeCreator comment.

Three things I'd want changed before merge, plus one naming decision I'd rather put to you and the maintainers than assert.

1. ITypeVisitor<TResult> has no fallback

All 25 overloads are required, so adding a kind is a source-breaking change for every implementor — and kinds are still coming. Missing today: UserDefined (not deprecated, and needed for extensions), NSTRUCT, and the deprecated timestamp/timestamp_tz/time. Skipping the deprecated three is defensible for construction, but they still appear on the wire, so you'll need them if this ever deserializes existing plans. Worth noting the PR description says "every Simple/Compound type kind", which overstates it slightly.

Default interface implementations fix the extensibility problem for free right now, and it's breaking to retrofit later. Suggestion inline on the file.

2. No tests for the type system

Struct.Equals/GetHashCode and Func.Equals/GetHashCode are the only hand-written non-trivial logic in the PR, and they're the only part untested. Starter file in a follow-up comment below. One of those tests fails against the current code and pins a real bug — see the Struct.cs comment.

3. CI has never run on this PR

gh pr checks 5 shows only the pending CLA, so net10.0 + Grpc.Tools 2.83.0 + a 57-minor-version proto jump is currently unverified by anything.

Related, and worth knowing before you rely on a green build: Substrait.Core.csproj:11 sets CompileOutputs="false", so the protos are generated but never compiled into the assembly. That means (a) dotnet build cannot catch a breaking proto change, so the submodule bump is unvalidated by construction; (b) the Google.Protobuf reference is unused at compile time; (c) every <see cref="Protobuf.XxxRel"/> in Relation/* points at a type absent from the compilation, and since GenerateDocumentationFile is off, nothing warns. All pre-existing — but if the plan is to flip CompileOutputs to true soon, better to find out now whether v0.99 protos actually compile.

4. A question about the Substrait.Core.Type namespace

One consequence that I don't think is obvious up front: C# resolves a simple name against enclosing namespace members before using imports, so within Substrait.* the name Type now binds to the namespace rather than System.Type. That makes this a compile error in the new test project, because Substrait.Core.Tests sits inside Substrait.Core:

namespace Substrait.Core.Tests;

public class Demo
{
  // CS0118: 'Type' is a namespace but is used like a type
  private readonly List<Type> _kinds = [];
}

If you'd rather confirm the lookup behaviour than take my word for it: paste that into the test project and run dotnet build. Thirty seconds, and it either errors or it doesn't.

Scope is narrower than I first assumed, so to be clear about what this is not: it doesn't reach consumers, because using imports types and not nested namespaces — an app in its own namespace never sees Type shadowed. It affects this library, the test project, and any future sibling project under Substrait.*, which I'd guess includes the proto conversion layer. The String/Decimal shadowing that forced the aliases atop ITypeVisitor.cs and TypeCreator.cs is separate and mostly cosmetic — the string/decimal keywords are immune, and that's what most code uses.

Options as I see them:

1. Substrait.Core.Types — one-word change, removes the hazard entirely, keeps every type name as-is.
2. Keep Type, absorb the cost — reasonable if cross-language consistency matters more; the other implementations use type as the package/module name (io.substrait.type in substrait-java), and matching them has real value for anyone reading two bindings side by side. Cost is System.Type fully qualified in this repo forever. Note global using aliases won't help consumers — they don't cross assembly references, so each project declares its own.
3. Suffix the type names (ListType, DecimalType) — orthogonal to the above, and lower stakes given the keyword immunity noted above.

I lean 1. If matching the other bindings' package naming is deliberate here, 2 is defensible and I'd drop it. Either way I'd rather settle it before the type system lands than after, since it's a namespace change on a public API.

On scope

45 files mixing repo hygiene, a submodule bump, a breaking namespace rename, and a new feature makes a future bisect painful, and the PR claims to close #3 (CI only) while doing considerably more. Your commits already separate cleanly — splitting into CI + editorconfig + slnx / TFM + submodule / namespace rename / type system would let the uncontroversial 80% land immediately. Also worth noting the branch is your fork's main, which will make follow-up rebases awkward.

Detailed comments inline. (Minor: "namsepacing" typo in the description.)

[nielspardon]

A starting point for point 2 of my review — test/Substrait.Core.Tests/TypeTests.cs. Three notes:

• Struct_IsNotMutatedByCallerOwnedArray fails against the current code. That's the defensive-copy bug from my Struct.cs comment, as an executable demo rather than an assertion you have to trust.
• EveryTypeClass_HasAVisitorOverload is the one I'd most want kept — it fails the moment a kind is added without a visitor overload, which is how the missing UserDefined/NSTRUCT surfaced.
• KindNameVisitor assumes the VisitFallback suggestion on ITypeVisitor.cs is taken. If you don't take it, drop Accept_DispatchesOnTheRuntimeKind or spell out all 25 overloads; nothing else in the file depends on it.

Note that System.Type has to stay unnamed here — inside Substrait.Core.Tests, bare Type binds to the Substrait.Core.Type namespace, so everything goes through var. Live illustration of point 4.

using Substrait.Core.Type;
using Substrait.Core.Type.Compound;
using Substrait.Core.Type.Simple;
using Decimal = Substrait.Core.Type.Compound.Decimal;
using String = Substrait.Core.Type.Simple.String;

namespace Substrait.Core.Tests;

public class TypeCreatorTests
{
  [Fact]
  public void Required_ProducesNonNullableTypes() => Assert.False(TypeCreator.Required.I32.Nullable);

  [Fact]
  public void Nullable_ProducesNullableTypes() => Assert.True(TypeCreator.Nullable.I32.Nullable);

  [Fact]
  public void Of_ReturnsTheMatchingSingleton()
  {
    Assert.Same(TypeCreator.Nullable, TypeCreator.Of(nullable: true));
    Assert.Same(TypeCreator.Required, TypeCreator.Of(nullable: false));
  }

  [Fact]
  public void AsNullable_PreservesTypeParameters()
  {
    var required = (Decimal)TypeCreator.Required.Decimal(precision: 10, scale: 2);

    var nullable = (Decimal)TypeCreator.AsNullable(required);

    Assert.True(nullable.Nullable);
    Assert.Equal(10, nullable.Precision);
    Assert.Equal(2, nullable.Scale);
  }

  [Fact]
  public void AsNotNullable_RoundTripsToTheOriginal()
  {
    var original = TypeCreator.Required.VarChar(255);

    Assert.Equal(original, TypeCreator.AsNotNullable(TypeCreator.AsNullable(original)));
  }
}

public class TypeEqualityTests
{
  [Fact]
  public void Struct_WithEqualFields_IsEqualAndHashesEqually()
  {
    var a = TypeCreator.Required.Struct(TypeCreator.Required.I32, TypeCreator.Nullable.String);
    var b = TypeCreator.Required.Struct(TypeCreator.Required.I32, TypeCreator.Nullable.String);

    Assert.Equal(a, b);
    Assert.Equal(a.GetHashCode(), b.GetHashCode());
  }

  [Fact]
  public void Struct_WithReorderedFields_IsNotEqual()
  {
    var a = TypeCreator.Required.Struct(TypeCreator.Required.I32, TypeCreator.Required.I64);
    var b = TypeCreator.Required.Struct(TypeCreator.Required.I64, TypeCreator.Required.I32);

    Assert.NotEqual(a, b);
  }

  [Fact]
  public void Struct_DifferingOnlyInNullability_IsNotEqual()
  {
    var required = TypeCreator.Required.Struct(TypeCreator.Required.I32);
    var nullable = TypeCreator.Nullable.Struct(TypeCreator.Required.I32);

    Assert.NotEqual(required, nullable);
  }

  [Fact]
  public void Struct_IsNotMutatedByCallerOwnedArray()
  {
    var fields = new[] { TypeCreator.Required.I32 };
    var type = TypeCreator.Required.Struct(fields);
    var hashBefore = type.GetHashCode();

    fields[0] = TypeCreator.Required.I64;

    Assert.Equal(hashBefore, type.GetHashCode());
  }

  [Fact]
  public void Func_WithEqualSignatures_IsEqualAndHashesEqually()
  {
    var a = TypeCreator.Required.Func([TypeCreator.Required.I32], TypeCreator.Required.Bool);
    var b = TypeCreator.Required.Func([TypeCreator.Required.I32], TypeCreator.Required.Bool);

    Assert.Equal(a, b);
    Assert.Equal(a.GetHashCode(), b.GetHashCode());
  }

  [Fact]
  public void Func_WithDifferentReturnType_IsNotEqual()
  {
    var a = TypeCreator.Required.Func([TypeCreator.Required.I32], TypeCreator.Required.Bool);
    var b = TypeCreator.Required.Func([TypeCreator.Required.I32], TypeCreator.Required.I64);

    Assert.NotEqual(a, b);
  }

  [Fact]
  public void NestedTypes_CompareStructurally()
  {
    var a = TypeCreator.Required.Map(
      TypeCreator.Required.String,
      TypeCreator.Nullable.List(TypeCreator.Required.I32));
    var b = TypeCreator.Required.Map(
      TypeCreator.Required.String,
      TypeCreator.Nullable.List(TypeCreator.Required.I32));

    Assert.Equal(a, b);
    Assert.Equal(a.GetHashCode(), b.GetHashCode());
  }

  [Fact]
  public void DifferentKindsWithIdenticalShape_AreNotEqual() =>
    Assert.NotEqual(
      TypeCreator.Required.PrecisionTime(6),
      TypeCreator.Required.PrecisionTimestamp(6));
}

public class TypeVisitorTests
{
  [Fact]
  public void EveryTypeClass_HasAVisitorOverload()
  {
    var kinds = typeof(TypeClass)
      .Assembly.GetTypes()
      .Where(t => !t.IsAbstract && t.IsSubclassOf(typeof(TypeClass)))
      .ToList();

    var covered = typeof(ITypeVisitor<>)
      .GetMethods()
      .Where(m => m.Name == "Visit")
      .Select(m => m.GetParameters()[0].ParameterType)
      .ToHashSet();

    var missing = kinds.Where(k => !covered.Contains(k)).Select(k => k.Name).Order().ToList();

    Assert.True(missing.Count == 0, $"No ITypeVisitor overload for: {string.Join(", ", missing)}");
  }

  [Fact]
  public void Accept_DispatchesOnTheRuntimeKind()
  {
    var visitor = new KindNameVisitor();

    Assert.Equal(nameof(Decimal), TypeCreator.Required.Decimal(10, 2).Accept(visitor));
    Assert.Equal(nameof(List), TypeCreator.Required.List(TypeCreator.Required.I32).Accept(visitor));
  }

  private sealed class KindNameVisitor : ITypeVisitor<string>
  {
    public string VisitFallback(TypeClass type) => type.GetType().Name;
  }
}

[nielspardon]

One more thing, and it's new information rather than a further review point — I should have flagged it upfront: I've been working on substrait-io/substrait-packaging#56 in parallel. It adds a csharp/ tree publishing Substrait.Protobuf, Substrait.Antlr and Substrait.Extensions to NuGet, built from the spec release. The intent is that substrait-csharp eventually drops the substrait submodule and its own Grpc.Tools codegen in favour of a PackageReference.

This shouldn't block anything here. It's still draft and gated on two org-side settings, and there's no release to depend on yet — so your v0.99 bump is the bridge until there is one, not wasted work.

It does put real evidence behind point 4 though, and I'd rather surface it before you decide than after. From proto/substrait/type.proto at v0.99.0:

8:option csharp_namespace = "Substrait.Protobuf";
17:message Type {

So Substrait.Protobuf.Type will be an actual class. Once a conversion layer exists it has to name both that class and the Substrait.Core.Type namespace in the same file — and because bare Type resolves to the namespace anywhere under Substrait.*, you could never write Type to mean the proto message. It'd be Substrait.Protobuf.Type or an alias, in every conversion file. That's the one place you'd most want the short name.

Adjacent trap while the names are still in play: if the conversion layer lands in Substrait.Core.Protobuf, then Protobuf binds to that namespace, and the <see cref="Protobuf.AggregateRel"/> docs in Relation/* quietly stop resolving.

Which is the nicer half of this — those crefs are dead today, per the CompileOutputs="false" note in my review, and they'd start resolving for free once a package reference lands. They were written against the right names all along.

Last thing, correcting myself: I suggested net8.0;net10.0 on the csproj. #56 ships netstandard2.0;net8.0, and that's the better thing to match — with netstandard2.0 in the dependency graph, net10-only is harder to justify than I argued.

Happy to coordinate on sequencing. And if you'd rather own the conversion layer yourself, say so and I'll stay clear of it.

[anasik]

• On UserDefined and NSTRUCT: deferred on purpose, not skipped by oversight. UserDefined depends on an anchor resolution registry that doesn't exist yet (the type_reference on the wire is just an
  integer pointing at the plan's extension declarations, and resolving it into something real needs machinery I haven't built). I'd rather design that properly once the extension system exists than redo it
  later. NSTRUCT is the spec's own words a "pseudo-type" ("Substrait's core type system is based entirely on ordinal positions, not named fields"), so it's not obviously a real wire level kind the same way
  Struct and Map are. Want to think about whether it deserves its own TypeClass at all before committing to a shape.

• On the deprecated time, timestamp, and timestamp_tz types: checked this against the actual spec history instead of assuming. They weren't just deprecated, they were fully removed in v0.88.0
  (2026-04-12). The changelog says it removes them "from: proto files, dialect schema, extension yamls, ANTLR grammar, test cases, coverage python code, documentation." We're pinned to v0.99.0, 11 releases
  past that removal, and there's no message definition left in type.proto, just three reserved field numbers with nothing behind them. This library won't have a usable release for a while either, so I don't
  see a realistic case for supporting them, and I'm leaving them out.

• On the conversion layer: happy to own it myself unless you're already working on it, would rather take it, or it's just more convenient on your end given the packaging work. No strong preference either way.

[anasik]

@nielspardon I also updated the PR description. And I have also added the missing tests that I had previously forgotten to git add and they were therefore never committed. But your suggestions definitely had more coverage than I already had. In a couple of cases your naming was better so I even renamed a couple of mine.