Ducklake migration #552

pskrbasu · 2025-09-09T11:13:53Z

Warning: DuckLake migration complete.
- Tables: 12/12 migrated (failed: 0, remaining: 0)
- Parquet files: 156/156 migrated (failed: 0, remaining: 0)
- Backup of migrated legacy data: '/Users/pskrbasu/.tailpipe/migration/migrated/default'

kaidaguerre

can't we just invoke migration from the root command rather than wrap every run func with withMigration?

added initial comments - we nned to think carefully about failures and rollback, and what state would be after potential failures

kaidaguerre · 2025-09-15T10:46:58Z

internal/database/convertor.go

 	}
-	// create the DuckDB table fpr this partition if it does not already exist
-	if err := w.ensureDuckLakeTable(w.Partition.TableName); err != nil {
+	slog.Error("schema", "name", w.tableSchema.Name)


this logging should be removed?

kaidaguerre · 2025-09-15T10:49:17Z

internal/database/convertor_ducklake.go


 // determine whether we have a ducklake table for this table, and if so, whether it needs schema updating
-func (w *Converter) ensureDuckLakeTable(tableName string) error {
+func EnsureDuckLakeTable(columns []*schema.ColumnSchema, db *DuckDb, tableName string) error {


rename file ducklake_table.go
are there still any convertor methods in this file? if so move to an existing convertor file

kaidaguerre · 2025-09-15T10:50:19Z

internal/database/tables.go


 	return schema, nil
 }
+


is this code copied from somehwere or written from scratch - if its a copy of existing code, mention that

This was old code which was used in inspect. Was removed in this version, so re-added them with 'legacy' named

kaidaguerre · 2025-09-15T10:50:54Z

internal/migration/migration.go

+	"github.com/turbot/tailpipe/internal/filepaths"
+)
+
+func MigrateDataToDucklake(ctx context.Context) error {


function comment

kaidaguerre · 2025-09-15T10:51:37Z

internal/migration/migration.go

+	// if the ~/.tailpipe/data directory has a .db file, it means that this is the first time we are migrating
+	// if the ~/.tailpipe/migration/migrating directory has a .db file, it means that this is a resume migration
+	initialMigration := hasTailpipeDb(dataDefaultDir)
+	continueMigration := hasTailpipeDb(migratingDefaultDir)


validate these are not both true

kaidaguerre · 2025-09-15T10:57:21Z

internal/migration/migration.go

+		if err := os.MkdirAll(config.GlobalWorkspaceProfile.GetMigratedDir(), 0755); err != nil {
+			return err
+		}
+		if err := utils.CopyFile(filepath.Join(dataDefaultDir, "tailpipe.db"), filepath.Join(config.GlobalWorkspaceProfile.GetMigratedDir(), "tailpipe.db")); err != nil {


should this move rather than copy to avoid the db file existing in 2 places? Otherwise if copy fails state will be confusing

Is MoveDirContents transactional? If it fails - will all files be in original location?

kaidaguerre · 2025-09-15T10:58:24Z

internal/migration/migration.go

+		}
+	}
+
+	return nil


do we handle/report parquet files for tables with no views?

kaidaguerre · 2025-09-15T10:59:26Z

internal/migration/migration.go

+		slog.Info("Found leaf node with parquet files", "table", tableName, "dir", dirPath, "files", filesInLeaf)
+
+		// Begin transaction
+		tx, err := db.BeginTx(ctx, nil)


maybe put the actual file migration into a separate func

kaidaguerre

not responded:
https://github.com/turbot/tailpipe/pull/552/files#r2348601278
https://github.com/turbot/tailpipe/pull/552/files#r2348623707
https://github.com/turbot/tailpipe/pull/552/files#r2348626270

kaidaguerre · 2025-09-15T14:37:59Z

internal/cmdconfig/cmd_hooks.go

 	// set the max memory if specified
 	setMemoryLimit()

+	// migrate legacy data to ducklake


better comment - mention tailpipe version numbers and native parquet vs ducklake

kaidaguerre · 2025-09-15T14:39:19Z

internal/cmdconfig/cmd_hooks.go

+	sigCtx, stop := signal.NotifyContext(baseCtx, syscall.SIGINT, syscall.SIGTERM)
+	// store stop so we can cancel after command completes (postRunHook)
+	signalStopFn = stop
+	cmd.SetContext(sigCtx)


what is this doing? why do we need it?

not needed. removednot needed. removed

kaidaguerre · 2025-09-15T14:39:53Z

internal/cmdconfig/cmd_hooks.go

+	err := migration.MigrateDataToDucklake(sigCtx)
+	if error_helpers.IsContextCancelledError(err) {
+		// suppress Cobra's usage/errors only for this cancelled invocation
+		cmd.SilenceUsage = true


what is this?

Cobra prints usage when a command returns an error. My cancellation returns an error (context cancelled) from preRun, so Cobra assumes “user error” and shows help.

The conditional snippet sets cmd.SilenceUsage = true and cmd.SilenceErrors = true only for cancellation, telling Cobra “don’t print usage or re-print the error.” Without it, you get the usage dump.

kaidaguerre · 2025-09-15T14:41:18Z

internal/cmdconfig/cmd_hooks.go

 		select {
 		case <-time.After(100 * time.Millisecond):
 			tasksCancelFn()
+			// ensure we cleanup signal context


better comment - whet is signalContext? why do we need this mechanism?

not needed. removed

kaidaguerre · 2025-09-15T14:43:04Z

internal/migration/migration.go

+
+	// validate: both should not be true - return that last migration left things in a bad state
+	if initialMigration && continueMigration {
+		return fmt.Errorf("Bad state - migration cannot proceed because both data and migrating directories contain tailpipe.db files.")


do we mention contacting support when we report this error?

kaidaguerre · 2025-09-15T14:44:34Z

internal/migration/migration.go

+
+	// STEP 3: If this is the first time we are migrating(tables in ~/.tailpipe/data) then move the whole contents of data dir
+	// into ~/.tailpipe/migration/migrating respecting the same folder structure.
+	// First-run: copy tailpipe.db into migrated immediately, then move data contents into migrating


this comment is out of date? instead just mention thast the process is transactional - there will only event be a single copy of db file and it will be wherever the data files are...

kaidaguerre · 2025-09-15T14:46:06Z

internal/migration/migration.go

+	if err = os.MkdirAll(migratedDir, 0755); err != nil {
+		return err
+	}
+	if err = utils.CopyDir(dataDefaultDir, migratingDefaultDir); err != nil {


have more explicit comments about the ordering of this and the reason

internal/migration/migration.go

…g migration status. - Introduced function to pre-compute total parquet files across matched directories. - Updated struct to track total, migrated, and failed parquet files. - Modified migration functions to utilize new status updates for parquet files during migration. - Improved spinner status messages to include parquet file migration progress.

addressed the requested changes

pskrbasu self-assigned this Sep 9, 2025

pskrbasu force-pushed the ducklake-migration branch 2 times, most recently from 4c7ef1e to 551073c Compare September 15, 2025 06:27

pskrbasu marked this pull request as ready for review September 15, 2025 07:32

kaidaguerre requested changes Sep 15, 2025

View reviewed changes

kaidaguerre previously requested changes Sep 15, 2025

View reviewed changes

pskrbasu and others added 22 commits September 16, 2025 17:39

initial - migration

913db8f

cancellation and run for every command

8bbe727

cancellation and print summary

acee926

spinner and summary

10d3164

changes with rebase

4c97063

ducklake option - write

ebb01c4

updated status messages

0dbc44f

refactor

8bb557a

cleanup

8499cb4

lint

a5dd813

lint

73f626b

cleanup

8c572aa

initial revision

d673467

revision

62d6cd8

cleanup

a025370

use contexthelpers.StartCancelHandler

47ca1b9

add better comment (suppress cobra logs)

7fd2cbf

add better comments

5a19d75

tidy up context usage, cancellation handling and general structure

4f5e8ad

minor changes

0d2903f

pf v2.7.0-rc.1

beecf73

pskrbasu force-pushed the ducklake-migration branch from 7c65ea9 to beecf73 Compare September 16, 2025 12:13

refactor error handling in format commands to use a consistent exit code

c35f831

pskrbasu added 2 commits September 16, 2025 18:39

remove cancelhandlers from cmd; use from prerunhooks

885a2ad

sdk v0.9.3

a72ac0d

Priyanka-Chatterjee-2000 approved these changes Sep 16, 2025

View reviewed changes

pskrbasu merged commit cee6e42 into develop Sep 16, 2025
14 checks passed

pskrbasu deleted the ducklake-migration branch September 16, 2025 13:36

Ducklake migration #552

Ducklake migration #552

Uh oh!

Conversation

pskrbasu commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaidaguerre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaidaguerre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pskrbasu commented Sep 9, 2025 •

edited

Loading