-
Notifications
You must be signed in to change notification settings - Fork 873
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| storage?: string | ||
| path: string | ||
| } | ||
| extra_args?: string | ||
| }[] | ||
| } | ||
|
|
||
|
|
@@ -42,15 +43,16 @@ | |
|
|
||
| s.ducklakes[ducklake.name] = { | ||
| catalog: ducklake.catalog, | ||
| storage: ducklake.storage | ||
| storage: ducklake.storage, | ||
| extra_args: ducklake.extra_args || undefined | ||
| } | ||
| } | ||
| return s | ||
| } | ||
| </script> | ||
|
|
||
| <script> | ||
| import { Plus } from 'lucide-svelte' | ||
| import { Plus, SettingsIcon } from 'lucide-svelte' | ||
|
|
||
| import Button from '../common/button/Button.svelte' | ||
|
|
||
|
|
@@ -81,6 +83,7 @@ | |
| import { isCustomInstanceDbEnabled } from './utils.svelte' | ||
| import { resource } from 'runed' | ||
| import CustomInstanceDbSelect from './CustomInstanceDbSelect.svelte' | ||
| import Label from '../Label.svelte' | ||
|
|
||
| const DEFAULT_DUCKLAKE_CATALOG_NAME = 'ducklake_catalog' | ||
|
|
||
|
|
@@ -316,29 +319,56 @@ | |
| </div> | ||
| </Cell> | ||
| <Cell class="w-12"> | ||
| {#if ducklakeIsDirty[ducklake.name]} | ||
| <Popover | ||
| openOnHover | ||
| contentClasses="p-2 text-sm text-secondary italic" | ||
| class="cursor-not-allowed" | ||
| > | ||
| <div class="flex gap-2"> | ||
| <Popover contentClasses="p-4" enableFlyTransition closeOnOtherPopoverOpen> | ||
| <svelte:fragment slot="trigger"> | ||
| <ExploreAssetButton | ||
| class="h-9" | ||
| asset={{ kind: 'ducklake', path: ducklake.name }} | ||
| {dbManagerDrawer} | ||
| disabled | ||
| /> | ||
| <div class="relative"> | ||
| <Button variant="default" iconOnly size="sm" endIcon={{ icon: SettingsIcon }} /> | ||
| {#if ducklake.extra_args} | ||
| <div | ||
| class="absolute -top-0.5 -right-0.5 w-2 h-2 bg-accent rounded-full border border-surface" | ||
| ></div> | ||
| {/if} | ||
| </div> | ||
| </svelte:fragment> | ||
| <svelte:fragment slot="content"> | ||
| <Label | ||
| label="Extra args" | ||
| tooltip="Additional arguments to pass in the ATTACH command. The argument list is substituted as-is. Separate them with commas." | ||
| > | ||
| <TextInput | ||
| bind:value={ducklake.extra_args} | ||
| class="min-w-96" | ||
| underlyingInputEl="textarea" | ||
| inputProps={{ placeholder: "METADATA_SCHEMA 'schema', ENCRYPTED true" }} | ||
| /> | ||
| </Label> | ||
| </svelte:fragment> | ||
| <svelte:fragment slot="content">Please save settings first</svelte:fragment> | ||
| </Popover> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| {:else} | ||
| <ExploreAssetButton | ||
| class="h-9" | ||
| asset={{ kind: 'ducklake', path: ducklake.name }} | ||
| {dbManagerDrawer} | ||
| /> | ||
| {/if} | ||
| {#if ducklakeIsDirty[ducklake.name]} | ||
| <Popover | ||
| openOnHover | ||
| contentClasses="p-2 text-sm text-secondary italic" | ||
| class="cursor-not-allowed" | ||
| > | ||
| <svelte:fragment slot="trigger"> | ||
| <ExploreAssetButton | ||
| class="h-9" | ||
| asset={{ kind: 'ducklake', path: ducklake.name }} | ||
| {dbManagerDrawer} | ||
| disabled | ||
| /> | ||
| </svelte:fragment> | ||
| <svelte:fragment slot="content">Please save settings first</svelte:fragment> | ||
| </Popover> | ||
| {:else} | ||
| <ExploreAssetButton | ||
| class="h-9" | ||
| asset={{ kind: 'ducklake', path: ducklake.name }} | ||
| {dbManagerDrawer} | ||
| /> | ||
| {/if} | ||
| </div> | ||
| </Cell> | ||
| <Cell class="w-12"> | ||
| <CloseButton small on:close={() => removeDucklake(ducklakeIndex)} /> | ||
|
|
||
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_argsafter theOVERRIDE_DATA_PATHhandling 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:
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.