feat: add DynamicParameter expression and plan bindings

[bvolpato]

Summary

Add full support for DynamicParameter expressions and plan-level DynamicParameterBinding, enabling parameterized queries in substrait-go.

This mirrors the functionality recently added in substrait-java#752, ensuring Go consumers can create and read plans containing dynamic parameters.

Changes

Expression (expr/expression.go)

• New DynamicParameter struct implementing the full Expression interface (String, ToProto, Equals, Visit, IsScalar, GetType, ToProtoFuncArg)
• Proto deserialization in ExprFromProto() with nil safety check

Builder (expr/builder.go)

• ExprBuilder.DynamicParam(outputType, paramRef) method with dynamicParamBuilder
• Implements BuildExpr() and BuildFuncArg() for use as function arguments

Plan bindings (plan/common.go, plan/plan.go, plan/builders.go)

• DynamicParameterBinding struct mapping parameter anchors to literal values
• Plan.ParameterBindings() accessor
• FromProto() / ToProto() serialization for parameter bindings
• Builder.PlanWithBindings() method on the Builder interface

Testing

17 new tests across two test files:

• expr/dynamic_parameter_test.go — 11 tests covering basic construction, nullability, equality, visit, proto roundtrip (5 types), nil proto error, builder (3 cases), builder as function argument, and multiple parameters
• plan/dynamic_parameter_test.go — 6 tests covering filter plan with bindings, project plan, multiple bindings, no bindings, JSON parsing roundtrip, and end-to-end builder usage

All existing tests continue to pass with zero regressions.

[codecov]

Codecov Report

❌ Patch coverage is 86.77686% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.56%. Comparing base (9478e8b) to head (db9f3f7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
plan/common.go	77.55%	9 Missing and 2 partials ⚠️
expr/expression.go	84.84%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
+ Coverage   68.33%   68.56%   +0.23%     
==========================================
  Files          47       47              
  Lines       10727    10843     +116     
==========================================
+ Hits         7330     7435     +105     
- Misses       3043     3053      +10     
- Partials      354      355       +1     

☔ View full report in Codecov by Sentry.
📢 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.

[benbellick]

Left a few comments but had to run. I think there is opportunity to simplify the tests a little bit so that it is easier to determine what they are doing with just a skim, e.g. making aroundtrip tests simply assert that the plan equals itself post-rountrip without any other checks.

I think the more important question for me is: do we want to validate that the types of dynamic parameters match the types of dynamic parameter values in the plan? This would probably be valuable. And if we aren't going to do it, probably makes sense to at least document that somewhere. (Sorry if you did this and I just missed it).

Anyways, thanks for doing this!

[benbellick]

Is this syntax part of the core substrait spec? I don't see it on the core docs here at least.

[bvolpato]

Nope, added a clarification:

// The $N:type String() format (e.g. "$0:i32") is an internal debugging
// representation used by this library; it is not part of the Substrait spec.

[benbellick]

can we just add these two as test cases to TestDynamicParameterToProtoRoundtrip? I think that that covers these tests and makes it a bit easier to follow all of the tests.

[bvolpato]

Done db9f3f7 ty!

[benbellick]

Can we drop this in either plan/testdata or expr/testdata?

[bvolpato]

Done db9f3f7 ty!

[benbellick]

LGTM, just added some claude-assisted comments about ways to make the test intention more clear. Feel free to ignore if you disagree. Thanks!

[benbellick]

I asked claude to help reduce the size of these tests to make them a bit easier to follow and it produced this. What do you think? I think its a bit easier to see the intended test.

// SPDX-License-Identifier: Apache-2.0

package expr_test

import (
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"github.com/substrait-io/substrait-go/v7/expr"
	"github.com/substrait-io/substrait-go/v7/extensions"
	"github.com/substrait-io/substrait-go/v7/types"
	proto "github.com/substrait-io/substrait-protobuf/go/substraitpb"
	pb "google.golang.org/protobuf/proto"
)

func TestDynamicParameterEquals(t *testing.T) {
	i64Req := &types.Int64Type{Nullability: types.NullabilityRequired}
	fp64Req := &types.Float64Type{Nullability: types.NullabilityRequired}

	base := &expr.DynamicParameter{OutputType: i64Req, ParameterReference: 0}

	tests := []struct {
		name  string
		other expr.Expression
		want  bool
	}{
		{"same type and ref", &expr.DynamicParameter{OutputType: i64Req, ParameterReference: 0}, true},
		{"different ref", &expr.DynamicParameter{OutputType: i64Req, ParameterReference: 1}, false},
		{"different type", &expr.DynamicParameter{OutputType: fp64Req, ParameterReference: 0}, false},
		{"different expression kind", expr.NewPrimitiveLiteral(int64(42), false), false},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			assert.Equal(t, tt.want, base.Equals(tt.other))
		})
	}
}

func TestDynamicParameterVisit(t *testing.T) {
	dp := &expr.DynamicParameter{
		OutputType:         &types.Int32Type{Nullability: types.NullabilityRequired},
		ParameterReference: 5,
	}

	visited := dp.Visit(func(e expr.Expression) expr.Expression { return e })
	assert.Same(t, dp, visited, "Visit should return same pointer for leaf expression")
}

func TestDynamicParameterToProtoRoundtrip(t *testing.T) {
	tests := []struct {
		name string
		dp   *expr.DynamicParameter
	}{
		{"required i32", &expr.DynamicParameter{
			OutputType: &types.Int32Type{Nullability: types.NullabilityRequired}, ParameterReference: 0}},
		{"nullable string", &expr.DynamicParameter{
			OutputType: &types.StringType{Nullability: types.NullabilityNullable}, ParameterReference: 1}},
	}

	reg := expr.NewEmptyExtensionRegistry(extensions.GetDefaultCollectionWithNoError())

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			assert.True(t, tt.dp.IsScalar())
			assert.True(t, tt.dp.GetType().Equals(tt.dp.OutputType))

			protoExpr := tt.dp.ToProto()
			require.NotNil(t, protoExpr)

			fromProto, err := expr.ExprFromProto(protoExpr, nil, reg)
			require.NoError(t, err)
			assert.True(t, tt.dp.Equals(fromProto), "roundtrip should produce equal expression")

			protoRoundTrip := fromProto.ToProto()
			assert.True(t, pb.Equal(protoExpr, protoRoundTrip), "proto roundtrip should be equal")
		})
	}
}

func TestDynamicParameterFromProtoNilDynamicParam(t *testing.T) {
	protoExpr := &proto.Expression{
		RexType: &proto.Expression_DynamicParameter{
			DynamicParameter: nil,
		},
	}

	_, err := expr.ExprFromProto(protoExpr, nil, expr.NewEmptyExtensionRegistry(extensions.GetDefaultCollectionWithNoError()))
	require.Error(t, err)
	assert.Contains(t, err.Error(), "dynamic parameter is nil")
}

func TestDynamicParameterBuilderNilType(t *testing.T) {
	b := expr.ExprBuilder{
		Reg: expr.NewEmptyExtensionRegistry(extensions.GetDefaultCollectionWithNoError()),
	}

	_, err := b.DynamicParam(nil, 0).BuildExpr()
	require.Error(t, err)
	assert.Contains(t, err.Error(), "dynamic parameter must have an output type")
}

func TestDynamicParameterBuilderAsFuncArg(t *testing.T) {
	b := expr.ExprBuilder{
		Reg:        expr.NewEmptyExtensionRegistry(extensions.GetDefaultCollectionWithNoError()),
		BaseSchema: types.NewRecordTypeFromStruct(boringSchema.Struct),
	}

	dpBuilder := b.DynamicParam(&types.Int8Type{Nullability: types.NullabilityRequired}, 0)

	e, err := b.ScalarFunc(addID).Args(
		dpBuilder,
		b.Wrap(expr.NewLiteral(int8(5), false)),
	).BuildExpr()
	require.NoError(t, err)
	assert.Contains(t, e.String(), "$0:i8")
}

func TestDynamicParameterTypeMismatchInFunction(t *testing.T) {
	b := expr.ExprBuilder{
		Reg:        expr.NewEmptyExtensionRegistry(extensions.GetDefaultCollectionWithNoError()),
		BaseSchema: types.NewRecordTypeFromStruct(boringSchema.Struct),
	}

	tests := []struct {
		name   string
		funcID extensions.ID
		dpType types.Type
		lit    func() (expr.Literal, error)
	}{
		{
			name:   "i32 where i8 expected",
			funcID: extensions.ID{URN: extensions.SubstraitDefaultURNPrefix + "functions_arithmetic", Name: "add:i8_i8"},
			dpType: &types.Int32Type{Nullability: types.NullabilityRequired},
			lit:    func() (expr.Literal, error) { return expr.NewLiteral(int8(5), false) },
		},
		{
			name:   "string where numeric expected",
			funcID: addID,
			dpType: &types.StringType{Nullability: types.NullabilityRequired},
			lit:    func() (expr.Literal, error) { return expr.NewLiteral(int32(5), false) },
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			_, err := b.ScalarFunc(tt.funcID).Args(
				b.DynamicParam(tt.dpType, 0),
				b.Wrap(tt.lit()),
			).BuildExpr()
			require.Error(t, err)
		})
	}
}

[benbellick]

What do you think of just simplifying this to only be roundtrip tests from JSON?

// SPDX-License-Identifier: Apache-2.0

package plan_test

import (
	"fmt"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"github.com/substrait-io/substrait-go/v7/extensions"
	"github.com/substrait-io/substrait-go/v7/plan"
	substraitproto "github.com/substrait-io/substrait-protobuf/go/substraitpb"
	"google.golang.org/protobuf/encoding/protojson"
	"google.golang.org/protobuf/proto"
)

func TestDynamicParameterPlanRoundtrip(t *testing.T) {
	for _, name := range []string{
		"dynamic_parameter_plan",
		"dynamic_parameter_filter", // this would have to be added
	} {
		t.Run(name, func(t *testing.T) {
			planJSON, err := testdata.ReadFile(fmt.Sprintf("testdata/%s.json", name))
			require.NoError(t, err)

			var protoPlan substraitproto.Plan
			require.NoError(t, protojson.Unmarshal(planJSON, &protoPlan))

			p, err := plan.FromProto(&protoPlan, extensions.GetDefaultCollectionWithNoError())
			require.NoError(t, err)

			backToProto, err := p.ToProto()
			require.NoError(t, err)
			assert.Truef(t, proto.Equal(&protoPlan, backToProto),
				"expected: %s\ngot: %s",
				protojson.Format(&protoPlan), protojson.Format(backToProto))
		})
	}
}

[benbellick]

Suggested change

	// type-checking if needed.
	// type-checking if needed.
	// TODO(#216): Validate that binding and declaration match

[CLAassistant]

All committers have signed the CLA.