Skip to content

Implement mode readwrite for Motherduck and DuckLake #7735

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion docs/docs/reference/project-files/connectors.md
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ allow_host_access: true # Allow host-level file path ac
---
type: connector # Must be `connector` (required)
driver: duckdb # Must be `duckdb` _(required)_

mode: "read" # Operation mode: `read` or `readwrite` _(default: read)_

path: "md:my_db" # Path to your MD database

Expand Down
10 changes: 10 additions & 0 deletions runtime/drivers/duckdb/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import (
const (
poolSizeMin int = 2
poolSizeMax int = 5

modeReadOnly = "read"
modeReadWrite = "readwrite"
)

// config represents the DuckDB driver config
Expand All @@ -35,6 +38,8 @@ type config struct {
// Secrets is a comma-separated list of connector names to create temporary secrets for before executing models.
// The secrets are not created for read queries.
Secrets string `mapstructure:"secrets"`
// Mode specifies the mode in which to open the database.
Mode string `mapstructure:"mode"`

// Path switches the implementation to use a generic rduckdb implementation backed by the db used in the Path
Path string `mapstructure:"path"`
Expand All @@ -55,6 +60,11 @@ func newConfig(cfgMap map[string]any) (*config, error) {
return nil, fmt.Errorf("could not decode config: %w", err)
}

// Validate mode if specified
if cfg.Mode != "" && cfg.Mode != modeReadOnly && cfg.Mode != modeReadWrite {
return nil, fmt.Errorf("invalid mode '%s': must be 'read' or 'readwrite'", cfg.Mode)
}

// Set pool size
poolSize := cfg.PoolSize
if poolSize == 0 && cfg.CPU != 0 {
Expand Down
41 changes: 41 additions & 0 deletions runtime/drivers/duckdb/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,44 @@ func Test_specialCharInPath(t *testing.T) {
require.NoError(t, res.Close())
require.NoError(t, conn.Close())
}

func TestModeConfigValidation(t *testing.T) {
tests := []struct {
name string
config map[string]any
expectError bool
}{
{
name: "valid read mode",
config: map[string]any{"mode": modeReadOnly},
expectError: false,
},
{
name: "valid readwrite mode",
config: map[string]any{"mode": modeReadWrite},
expectError: false,
},
{
name: "empty mode is valid",
config: map[string]any{},
expectError: false,
},
{
name: "invalid mode should fail",
config: map[string]any{"mode": "invalid"},
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := newConfig(tt.config)
if tt.expectError {
require.Error(t, err)
require.Contains(t, err.Error(), "invalid mode")
} else {
require.NoError(t, err)
}
})
}
}
31 changes: 31 additions & 0 deletions runtime/drivers/duckdb/duckdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ var motherduckSpec = drivers.Spec{
Placeholder: "my_new_source",
Required: true,
},
{
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be part of config properties and not source properties. The source properties apply when we are creating a source that extracts data from motherduck.

Key: "mode",
Type: drivers.StringPropertyType,
Required: false,
DisplayName: "Mode",
Description: "Set the mode for the DuckDB connection. By default, it is set to 'read' which allows only read operations. Set to 'readwrite' to enable model creation and table mutations.",
Placeholder: modeReadOnly,
Default: modeReadOnly,
NoPrompt: true,
},
},
}

Expand Down Expand Up @@ -155,6 +165,18 @@ func (d Driver) Open(instanceID string, cfgMap map[string]any, st *storage.Clien
olapSemSize = 1
}

// Set the mode for the connection
if cfg.Mode == "" {
// The default mode depends on the connection type:
// - For generic connections (Motherduck/DuckLake with Path/Attach), default to "read"
// - For connections using the embedded DuckDB, default to "readwrite" to maintain compatibility
if cfg.Path != "" || cfg.Attach != "" {
cfg.Mode = modeReadOnly
} else {
cfg.Mode = modeReadWrite
}
}

ctx, cancel := context.WithCancel(context.Background())
c := &connection{
instanceID: instanceID,
Expand Down Expand Up @@ -384,6 +406,11 @@ func (c *connection) AsObjectStore() (drivers.ObjectStore, bool) {

// AsModelExecutor implements drivers.Handle.
func (c *connection) AsModelExecutor(instanceID string, opts *drivers.ModelExecutorOptions) (drivers.ModelExecutor, bool) {
if opts.OutputHandle == c && c.config.Mode != modeReadWrite {
c.logger.Warn("Model execution is disabled. To enable modeling on this DuckDB database, set 'mode: readwrite' in your connector configuration. WARNING: This will allow Rill to create and overwrite tables in your database.")
return nil, false
}

if opts.InputHandle == c && opts.OutputHandle == c {
return &selfToSelfExecutor{c}, true
}
Expand Down Expand Up @@ -422,6 +449,10 @@ func (c *connection) AsModelExecutor(instanceID string, opts *drivers.ModelExecu

// AsModelManager implements drivers.Handle.
func (c *connection) AsModelManager(instanceID string) (drivers.ModelManager, bool) {
if c.config.Mode != modeReadWrite {
c.logger.Warn("Model execution is disabled. To enable modeling on this DuckDB database, set 'mode: readwrite' in your connector configuration. WARNING: This will allow Rill to create and overwrite tables in your database.")
return nil, false
}
return c, true
}

Expand Down
136 changes: 136 additions & 0 deletions runtime/drivers/duckdb/duckdb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,139 @@ func TestNoFatalErrConcurrent(t *testing.T) {
err = handle.Close()
require.NoError(t, err)
}

func TestDuckDBModeDefaults(t *testing.T) {
tests := []struct {
name string
config map[string]any
expectedMode string
}{
{
name: "embedded duckdb defaults to readwrite",
config: map[string]any{"dsn": ":memory:"},
expectedMode: modeReadWrite,
},
{
name: "external duckdb with path defaults to read",
config: map[string]any{"path": "/tmp/test.db"},
expectedMode: modeReadOnly,
},
{
name: "external duckdb with attach defaults to read",
config: map[string]any{"attach": "'/path/to/db.db' AS external"},
expectedMode: modeReadOnly,
},
{
name: "explicit readwrite mode",
config: map[string]any{"dsn": ":memory:", "mode": "readwrite"},
expectedMode: modeReadWrite,
},
{
name: "explicit read mode",
config: map[string]any{"dsn": ":memory:", "mode": "read"},
expectedMode: modeReadOnly,
},
{
name: "explicit readwrite mode overrides path default",
config: map[string]any{"path": "/tmp/test.db", "mode": "readwrite"},
expectedMode: modeReadWrite,
},
{
name: "motherduck defaults to readonly",
config: map[string]any{"path": "md:my_db"},
expectedMode: modeReadOnly,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg, err := newConfig(tt.config)
require.NoError(t, err)

// Apply default mode logic
if cfg.Mode == "" {
if cfg.Path != "" || cfg.Attach != "" {
cfg.Mode = modeReadOnly
} else {
cfg.Mode = modeReadWrite
}
}

require.Equal(t, tt.expectedMode, cfg.Mode)
})
}
}

func TestDuckDBModeEnforcement(t *testing.T) {
ctx := context.Background()

t.Run("read mode blocks model execution", func(t *testing.T) {
handle, err := drivers.Open("duckdb", "test", map[string]any{
"dsn": ":memory:",
"mode": "read",
}, storage.MustNew(t.TempDir(), nil), activity.NewNoopClient(), zap.NewNop())
require.NoError(t, err)
defer handle.Close()

// Test AsModelExecutor
opts := &drivers.ModelExecutorOptions{
InputHandle: handle,
OutputHandle: handle,
}
executor, ok := handle.AsModelExecutor("test", opts)
require.False(t, ok)
require.Nil(t, executor)

// Test AsModelManager
manager, ok := handle.AsModelManager("test")
require.False(t, ok)
require.Nil(t, manager)
})

t.Run("readwrite mode allows model execution", func(t *testing.T) {
handle, err := drivers.Open("duckdb", "test", map[string]any{
"dsn": ":memory:",
"mode": "readwrite",
}, storage.MustNew(t.TempDir(), nil), activity.NewNoopClient(), zap.NewNop())
require.NoError(t, err)
defer handle.Close()

// Test AsModelExecutor
opts := &drivers.ModelExecutorOptions{
InputHandle: handle,
OutputHandle: handle,
}
executor, ok := handle.AsModelExecutor("test", opts)
require.True(t, ok)
require.NotNil(t, executor)

// Test AsModelManager
manager, ok := handle.AsModelManager("test")
require.True(t, ok)
require.NotNil(t, manager)
})

t.Run("read mode allows reading", func(t *testing.T) {
handle, err := drivers.Open("duckdb", "test", map[string]any{
"dsn": ":memory:",
"mode": "read",
}, storage.MustNew(t.TempDir(), nil), activity.NewNoopClient(), zap.NewNop())
require.NoError(t, err)
defer handle.Close()

// Should still allow OLAP queries
olap, ok := handle.AsOLAP("test")
require.True(t, ok)
require.NotNil(t, olap)

// Test a simple query
res, err := olap.Query(ctx, &drivers.Statement{Query: "SELECT 1 as test"})
require.NoError(t, err)
require.True(t, res.Next())
var value int
err = res.Scan(&value)
require.NoError(t, err)
require.Equal(t, 1, value)
require.NoError(t, res.Close())
})
}
66 changes: 66 additions & 0 deletions runtime/drivers/duckdb/motherduck_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package duckdb_test

import (
"testing"

"github.com/rilldata/rill/runtime/drivers"
activity "github.com/rilldata/rill/runtime/pkg/activity"
"github.com/rilldata/rill/runtime/storage"
"github.com/rilldata/rill/runtime/testruntime"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
)

func TestMotherDuckModeEnforcement(t *testing.T) {
t.Run("read mode blocks model execution", func(t *testing.T) {
cfg := testruntime.AcquireConnector(t, "motherduck")
cfg["mode"] = "read"

handle, err := drivers.Open("motherduck", "test", cfg,
storage.MustNew(t.TempDir(), nil),
activity.NewNoopClient(),
zap.NewNop())
require.NoError(t, err)
defer handle.Close()

// Test AsModelExecutor
opts := &drivers.ModelExecutorOptions{
InputHandle: handle,
OutputHandle: handle,
}
executor, ok := handle.AsModelExecutor("test", opts)
require.False(t, ok)
require.Nil(t, executor)

// Test AsModelManager
manager, ok := handle.AsModelManager("test")
require.False(t, ok)
require.Nil(t, manager)
})

t.Run("readwrite mode allows model execution", func(t *testing.T) {
cfg := testruntime.AcquireConnector(t, "motherduck")
cfg["mode"] = "readwrite"

handle, err := drivers.Open("motherduck", "test", cfg,
storage.MustNew(t.TempDir(), nil),
activity.NewNoopClient(),
zap.NewNop())
require.NoError(t, err)
defer handle.Close()

// Test AsModelExecutor
opts := &drivers.ModelExecutorOptions{
InputHandle: handle,
OutputHandle: handle,
}
executor, ok := handle.AsModelExecutor("test", opts)
require.True(t, ok)
require.NotNil(t, executor)

// Test AsModelManager
manager, ok := handle.AsModelManager("test")
require.True(t, ok)
require.NotNil(t, manager)
})
}
Loading