-
Notifications
You must be signed in to change notification settings - Fork 862
Ducklake default extra_args #7509
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
Conversation
Deploying windmill with
|
| Latest commit: |
639ae88
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9831b4cb.windmill.pages.dev |
| Branch Preview URL: | https://di-ducklake-extra-arsg.windmill.pages.dev |
|
Claude finished @diegoimbert's task in 2m 22s —— View job PR Review: Ducklake default extra_argsSummaryThis PR adds support for configuring default Changes Overview
Code Quality ✅
No Issues Found
Minor Suggestions (Non-blocking)
Testing InstructionsNavigate to Workspace Settings > Ducklake section. Create or edit an existing ducklake configuration. Click the new settings icon (gear) that appears in the row to access the "Extra args" popover. Enter ducklake-specific arguments in the textarea (e.g., |
| format!("{},{}", extra_args, default_extra_args) | ||
| } else { | ||
| extra_args | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice implementation! The approach of appending default extra_args after the OVERRIDE_DATA_PATH handling is correct. The comment explaining the premise is helpful.
One minor consideration: if the user provides conflicting extra_args (e.g., the same argument is set in both the ducklake default and the inline ATTACH statement), DuckDB will either:
- Use the last value specified
- Throw an error for certain arguments
This is probably acceptable behavior since the user can always remove the default extra_args if they want to override them inline. Just something to note in documentation if not already covered.
| </Label> | ||
| </svelte:fragment> | ||
| <svelte:fragment slot="content">Please save settings first</svelte:fragment> | ||
| </Popover> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of a Popover with a settings icon to keep the main UI clean while providing access to advanced options. The implementation is clean and follows the project patterns well.
Minor UX suggestion: Consider adding a visual indicator (e.g., a small dot on the settings icon) when extra_args has a value, so users know at a glance that extra args are configured. This would help avoid surprises when someone modifies a ducklake without realizing it has default extra args set.
Uh oh!
There was an error while loading. Please reload this page.