Skip to content

feat: Set connector refs for models #8591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
k-anshul wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

feat: Set connector refs for models #8591

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aff9d75
set connector refs for models
k-anshul Jan 6, 2026
ce0379d
use post parse hooks
k-anshul Jan 7, 2026
2f3eb90
lint fix
k-anshul Jan 7, 2026
9029867
Merge remote-tracking branch 'origin/main' into connector_refs
k-anshul Jan 8, 2026
cc65e71
extract to common function
k-anshul Jan 8, 2026
233f3b4
connector ref logic change
k-anshul Jan 8, 2026
49f6ac9
handle resource updates
k-anshul Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion runtime/parser/parse_metrics_view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,8 @@ func (p *Parser) parseMetricsView(node *Node) error {
return err
}
node.Refs = append(node.Refs, securityRefs...)

// kind is unspecified because all connectors are not explicit and may not exist as resource, will be resolved later
node.Refs = append(node.Refs, ResourceName{Name: node.Connector, Kind: ResourceKindUnspecified})
Copy link
Contributor

@begelundmuller begelundmuller Jan 6, 2026

Choose a reason for hiding this comment

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

The node.Connector is auto-populated with the default connector if not set. That seems problematic in this case because the model (if it is an actual model, and not an external table) may have a different connector, which will actually be used here:

rill/runtime/reconcilers/metrics_view.go

Line 84 in 1b15e01

mv.Spec.Connector = res.GetModel().State.ResultConnector

The root of this issue comes from the annoying duplicate role of model: for pointing both to other models and to external tables. I'm not sure, but it may be necessary to pull some of that handling into the parser here instead to handle this correctly.

var cacheTTLDuration time.Duration
if tmp.Cache.KeyTTL != "" {
cacheTTLDuration, err = time.ParseDuration(tmp.Cache.KeyTTL)
Expand Down
6 changes: 6 additions & 0 deletions runtime/parser/parse_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ func (p *Parser) parseModel(ctx context.Context, node *Node) error {
inputProps = map[string]any{}
}

// kind is unspecified because all connectors are not explicit and may not exist as resource, will be resolved later
node.Refs = append(node.Refs, ResourceName{Name: inputConnector, Kind: ResourceKindUnspecified})
Copy link
Contributor

@begelundmuller begelundmuller Jan 6, 2026

Choose a reason for hiding this comment

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

Using unspecified may be problematic because we know it's a connector in this case, we just don't know if the connector exists or not. That is different from the inference use case because that can mean that e.g. a model with sql: SELECT * FROM slack may suddenly get a ref to a slack connector, which would be unexpected. I think the solution may be to rethink the inference a bit here.

I'm not sure, but here's a quick thought: maybe this problem and the previous problem could be fixed with post parse hooks? I.e. changing inferUnspecifiedRefs to applyPostParseHooks(r *Resource), and supporting post parse hooks in some way like this:

// After all resources have been parsed, check if the input connector exists as a resource.
// If it does, add it to the refs.
node.PostParseHooks = append(node.PostParseHooks, func (r *Resource) {
  n := ResourceName{ResourceKindConnector, inputConnector}
  if _, ok := p.Resources[n] {
    r.Refs = append(r.Refs, n)
  }
})

// ...
e, err := p.insertResource(ResourceKindExplore, node.Name, node.Paths, node.PostParseHooks)


// Special handling for adding SQL to the input properties
if sql := strings.TrimSpace(node.SQL); sql != "" {
refs, err := p.inferSQLRefs(node)
Expand Down Expand Up @@ -154,6 +157,9 @@ func (p *Parser) parseModel(ctx context.Context, node *Node) error {
if outputConnector == "" {
outputConnector = p.defaultOLAPConnector()
}

// kind is unspecified because all connectors are not explicit and may not exist as resource, will be resolved later
node.Refs = append(node.Refs, ResourceName{Name: outputConnector, Kind: ResourceKindUnspecified})
outputProps := tmp.Output.Properties

// Backwards compatibility: materialize can be specified outside of the output properties
Expand Down
8 changes: 6 additions & 2 deletions runtime/parser/parse_partial_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,15 @@ func (p *Parser) parseDataYAML(raw *DataYAML, contextualConnector string) (strin
count++
resolver = "sql"
resolverProps["sql"] = raw.SQL
var connector string
if raw.Connector != "" {
resolverProps["connector"] = raw.Connector
connector = raw.Connector
} else if contextualConnector != "" {
resolverProps["connector"] = contextualConnector
connector = contextualConnector
}
resolverProps["connector"] = connector
// kind is unspecified because all connectors are not explicit and may not exist as resource, will be resolved later
refs = append(refs, ResourceName{Kind: ResourceKindUnspecified, Name: connector})
}

// Handle metrics SQL resolver
Expand Down
9 changes: 8 additions & 1 deletion runtime/parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,14 @@ func (p *Parser) inferUnspecifiedRefs(r *Resource) {
}
}

// Rule 4: Skip it
// Rule 4: For any resource if there is a connector with that name, use it
n := ResourceName{Kind: ResourceKindConnector, Name: ref.Name}
if _, ok := p.Resources[n.Normalized()]; ok {
refs = append(refs, n)
continue
}

// Rule 5: Skip it
}

slices.SortFunc(refs, func(a, b ResourceName) int {
Expand Down
2 changes: 2 additions & 0 deletions runtime/parser/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ schema: default
{
Name: ResourceName{Kind: ResourceKindModel, Name: "s1"},
Paths: []string{"/sources/s1.yaml"},
Refs: []ResourceName{{Kind: ResourceKindConnector, Name: "s3"}},
ModelSpec: &runtimev1.ModelSpec{
InputConnector: "s3",
OutputConnector: "duckdb",
Expand All @@ -269,6 +270,7 @@ schema: default
{
Name: ResourceName{Kind: ResourceKindModel, Name: "s2"},
Paths: []string{"/sources/s2.sql"},
Refs: []ResourceName{{Kind: ResourceKindConnector, Name: "postgres"}},
ModelSpec: &runtimev1.ModelSpec{
InputConnector: "postgres",
OutputConnector: "duckdb",
Expand Down
Loading