-
Notifications
You must be signed in to change notification settings - Fork 689
feat: duckdb sql lang support #5761
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: |
ab52287
|
Status: | ✅ Deploy successful! |
Preview URL: | https://75eca8e3.windmill.pages.dev |
Branch Preview URL: | https://di-duckdb.windmill.pages.dev |
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.
Caution
Changes requested ❌
Reviewed everything up to cb60101 in 2 minutes and 35 seconds. Click for details.
- Reviewed
2341
lines of code in36
files - Skipped
0
files when reviewing. - Skipped posting
14
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/consts.ts:206
- Draft comment:
Ensure the DUCKDB_TYPES array covers all intended DuckDB type names and follows consistent casing with other SQL dialects. Consider adding inline documentation on any differences. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment suggests ensuring that theDUCKDB_TYPES
array covers all intended DuckDB type names and follows consistent casing with other SQL dialects. It also suggests adding inline documentation on any differences. This comment is asking the author to ensure something, which violates the rule against asking the author to ensure behavior is intended. However, it also provides a specific suggestion to add inline documentation, which could be useful. Overall, the comment is more about ensuring and confirming intentions, which is not allowed.
2. frontend/src/lib/infer.ts:153
- Draft comment:
For 'duckdb', errors from JSON.parse(parse_duckdb(code)) are not explicitly handled. Consider adding error handling or fallback messages for better troubleshooting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment appears incorrect because error handling already exists. The code follows a consistent pattern where JSON parse errors are caught at a higher level. The error handling at line 207-208 covers all language parsers including duckdb. Adding additional error handling would be redundant and inconsistent with how other database parsers are handled. I could be missing some duckdb-specific error cases that aren't covered by the global error handling. There might be a reason why duckdb needs special handling. The consistent pattern across all database parsers and the existing global error handling strongly suggests that no special error handling is needed for duckdb specifically. Delete the comment because error handling already exists at a higher level and the suggestion would make duckdb inconsistent with other parsers.
3. frontend/src/lib/script_helpers.ts:310
- Draft comment:
DUCKDB_INIT_CODE includes an ATTACH clause referencing a postgres resource. Verify that this is intentional and document why a DuckDB script attaches a Postgres resource. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking the author to verify their intention and document the reason for a specific code behavior. This violates the rule against asking the author to confirm their intention or to explain their changes. However, it does point out a potentially unusual behavior (attaching a Postgres resource in a DuckDB script), which could be useful if rephrased to suggest a review of the logic or to confirm the intention without asking for documentation.
4. frontend/src/lib/components/common/languageIcons/LanguageIcon.svelte:97
- Draft comment:
Check that the mapping for 'duckdb' correctly uses DuckDbIcon and displays the expected label. Consistency in naming and styling is key. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte:758
- Draft comment:
The tag filter array now excludes 'duckdb'. Confirm that this exclusion in job tag display is intentional and aligns with the overall UX for showing job language tags. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment asks for confirmation about an intentional change, which violates the review rules. The presence of "ADD_NEW_LANG" comment suggests this is exactly where new languages should be added. The change is straightforward - adding a new language tag to an existing list. There's no evidence of any issue. The "ADD_NEW_LANG" comment could be outdated or misleading. The list might serve some other purpose beyond just languages. Even if the list serves multiple purposes, asking for confirmation adds no value - the author clearly intended to add duckdb since they made the change. The comment provides no actionable feedback. Delete the comment. It only asks for confirmation of an intentional change, which violates the review rules. The change appears correct given the "ADD_NEW_LANG" comment.
6. backend/migrations/20250515084520_duckdb_support.up.sql:1
- Draft comment:
Typo: The comment on line 1 says "Add up migration script here". Consider rewording it to something like "Add duckdb up migration script here" or simply "Add duckdb support migration script" to improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This appears to be a standard migration comment template that's commonly used across migration files. The purpose of the migration is already clear from the filename and the actual SQL commands. Changing the comment template doesn't improve functionality and could make the migrations less consistent with others. Maybe having more specific migration comments could help developers quickly understand the purpose when browsing through migration files. The filename already contains 'duckdb_support' and the SQL is clear and concise. The standard comment template helps maintain consistency across migrations. Delete this comment as it suggests changing a standard template without adding significant value, and could reduce consistency across migration files.
7. backend/parsers/windmill-parser-sql/src/lib.rs:97
- Draft comment:
Typographical suggestion: In the error message at line 97, consider capitalizing 'sql' to 'SQL' for consistency with standard naming conventions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While SQL is typically written in uppercase as an acronym, this codebase consistently uses lowercase "sql" in all similar error messages. The comment would make this one error message inconsistent with all others. The comment is about style rather than functionality, and would introduce inconsistency rather than improve consistency. The comment is technically correct that SQL is conventionally written in uppercase as an acronym. Perhaps there's value in starting to use proper capitalization. While proper capitalization is good in general, introducing it in just one place would reduce consistency. If capitalization should be changed, it should be done across all error messages in a separate PR. Delete the comment. While technically correct about SQL capitalization, implementing it would reduce codebase consistency. Such a change should be done systematically across all error messages if desired.
8. backend/windmill-common/src/scripts.rs:49
- Draft comment:
Typo/lexicographical note: Consider whether the variant should be "DuckDB" rather than "DuckDb" for consistency with other database-related identifiers (e.g., OracleDB). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment points out an inconsistency with OracleDB, there isn't actually a consistent pattern across database names in the enum. Some use DB suffix (OracleDB), others don't (Mysql, Postgresql). The casing varies too. Without a clear established pattern, enforcing consistency with just OracleDB seems arbitrary. The current DuckDb name follows valid Rust enum variant naming conventions. I could be wrong about dismissing consistency - maybe OracleDB represents the intended style that other variants should follow. The comment might be highlighting a valid opportunity to standardize naming. Given the wide variety in existing database name casing (Postgresql, Mysql, Bigquery, etc), there doesn't seem to be a strong precedent for OracleDB's style being the standard. Making this single change wouldn't meaningfully improve consistency. The comment should be deleted. While it points out an inconsistency, there's no clear established pattern for database name casing in this enum, so enforcing consistency with just OracleDB is not clearly beneficial.
9. backend/windmill-worker/Cargo.toml:38
- Draft comment:
Typo found: In the inline comment on line 38, 'audit-worth actions' might be intended to be 'audit-worthy actions'. Please clarify and correct if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-worker/src/worker.rs:3051
- Draft comment:
The new enum variant 'ScriptLang::DuckDb' uses a lowercase 'b' whereas similar variants (like 'OracleDB') have a different capitalization. Consider if it should be 'DuckDB' for consistency. - Reason this comment was not posted:
Marked as duplicate.
11. cli/metadata.ts:533
- Draft comment:
Typographical suggestion: Consider renaming 'inferedSchema' to 'inferredSchema' if the former isn't intentional. This change would improve readability. - Reason this comment was not posted:
Comment was on unchanged code.
12. frontend/src/lib/infer.ts:155
- Draft comment:
Typographical error: It looks likeinferedSchema
might be intended to beinferredSchema
. Consider correcting to improve readability. - Reason this comment was not posted:
Comment was on unchanged code.
13. frontend/src/lib/infer.ts:202
- Draft comment:
Typo: The variable name 'inferedSchema' appears to be misspelled. Consider renaming it to 'inferredSchema' for clarity and consistency. - Reason this comment was not posted:
Comment was on unchanged code.
14. frontend/src/lib/script_helpers.ts:320
- Draft comment:
Typo: There appears to be an extra space before the exclamation mark in the string (' new friends !'); consider changing it to 'new friends!' for a cleaner output. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a very minor formatting suggestion that doesn't affect functionality. The comment is about code that was changed in the diff, but it's purely about string formatting/aesthetics rather than logic or functionality. The rules state not to make comments that are obvious or unimportant. The space could be intentional for readability in the SQL output. Also, this is a demo/example code block, so perfect formatting may not be critical. Even if intentional, this kind of minor formatting detail doesn't warrant a comment. The current format is perfectly readable and functional. Delete the comment as it addresses a trivial formatting issue that doesn't impact functionality or code quality in any meaningful way.
Workflow ID: wflow_isOiqU1HxoBC3jcq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -349,7 +349,7 @@ pub fn should_validate_schema(code: &str, lang: &ScriptLang) -> bool { | |||
let comment = match lang { | |||
Nativets | Bun | Bunnative | Deno | Php | CSharp | Java => "//", | |||
Python3 | Go | Bash | Powershell | Graphql | Ansible | Nu => "#", | |||
Postgresql | Mysql | Bigquery | Snowflake | Mssql | OracleDB => "--", | |||
Postgresql | Mysql | Bigquery | Snowflake | Mssql | OracleDB | DuckDb => "--", |
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.
Typo/consistency: The new constant DuckDb
might be better written as DuckDB
to align with the naming convention used for similar identifiers (e.g., OracleDB
).
Postgresql | Mysql | Bigquery | Snowflake | Mssql | OracleDB | DuckDb => "--", | |
Postgresql | Mysql | Bigquery | Snowflake | Mssql | OracleDB | DuckDB => "--", |
}; | ||
|
||
let bq_credentials_path = make_bq_credentials_path(&job.id); | ||
env::set_var("GOOGLE_APPLICATION_CREDENTIALS", &bq_credentials_path); |
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.
suspicious that this is always set
) | ||
.to_string(); | ||
Ok(vec![ | ||
"INSTALL bigquery FROM community;".to_string(), |
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.
shouldn't we do that only once (as in verify it hasn't already been set) or is it idempotent?
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.
It's only necessary once but didn't bother because negligible performance impact. Also that's the way we already do it with the S3 connection settings helper which includes the load and install httpfs statements every time
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.
minor nits
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.
Important
Looks good to me! 👍
Reviewed 7ec8053 in 1 minute and 44 seconds. Click for details.
- Reviewed
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-worker/src/duckdb_executor.rs:210
- Draft comment:
Global env var manipulation: Removing the GOOGLE_APPLICATION_CREDENTIALS right before file deletion is risky in a concurrent environment. Env vars are process-wide and can cause race conditions if multiple jobs run concurrently. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment raises a valid concern about process-wide env vars, looking deeper: 1. The env var is only used during the BigQuery query execution which happens in a single blocking task 2. The env var is set right before the BigQuery attachment and removed right after query completion 3. The job ID is unique per execution, so the credentials file path is unique 4. The code appears to be designed to handle one query at a time in this process I could be wrong about the process architecture - maybe multiple queries do run concurrently in the same process. The comment author may have more context about the runtime environment. However, even if concurrent queries are possible, the comment doesn't provide a clear actionable solution. The env var appears necessary for the BigQuery extension to work, and the code already uses job-specific credential files. The comment raises a theoretical issue but doesn't provide clear evidence of an actual problem or actionable solution. The current implementation appears reasonably safe given the constraints.
2. backend/windmill-worker/src/duckdb_executor.rs:536
- Draft comment:
Setting GOOGLE_APPLICATION_CREDENTIALS in transform_attach_db_resource_query is a global side-effect that may interfere with concurrent jobs. Consider a thread-safe alternative to passing credentials. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% While environment variables are global, the code appears to handle this carefully: 1. Each job uses a unique file path based on job_id 2. The variable is cleaned up after use in do_duckdb() 3. The DuckDB connection is created in a spawn_blocking task, isolating it 4. The variable is only used during the DuckDB connection setup This suggests the concurrent job interference risk is minimal. The comment raises a valid concern about global state. Even with cleanup, there could be a race condition where one job overwrites another's credentials between the set and use. However, the DuckDB connection is created immediately after setting the variable in a blocking task, and the variable is only used during connection setup. The window for interference is extremely small. While the comment raises a theoretically valid concern, the actual risk is minimal due to the code's careful handling. The current implementation is a reasonable tradeoff given DuckDB's requirements.
Workflow ID: wflow_CAvujym5zw5ojwIA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
"float" | "float4" | "real" => duckdb::types::Value::Float(v as f32), | ||
"double" | "float8" => duckdb::types::Value::Double(v), | ||
"decimal" | "numeric" => { | ||
duckdb::types::Value::Decimal(Decimal::from_f64(v).unwrap()) |
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.
The unwrap()
call on Decimal::from_f64(v)
could cause a panic if the f64 value cannot be represented as a Decimal. Consider using ok_or_else()
to convert this to a proper error that can be propagated:
duckdb::types::Value::Decimal(
Decimal::from_f64(v)
.ok_or_else(|| Error::ExecutionErr("Could not convert f64 to Decimal".to_string()))?
)
This maintains the error handling pattern used elsewhere in this function.
duckdb::types::Value::Decimal(Decimal::from_f64(v).unwrap()) | |
duckdb::types::Value::Decimal( | |
Decimal::from_f64(v).ok_or_else(|| | |
Error::ExecutionErr("Could not convert f64 to Decimal".to_string()) | |
)? | |
) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
Important
Looks good to me! 👍
Reviewed 91764a3 in 1 minute and 4 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/package.json:4
- Draft comment:
Bump of windmill-parser-wasm-regex from 1.481.0 to 1.492.1: ensure compatibility with duckdb SQL support and add/regress tests if needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_3DpjzzuQqJ2GqhNO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -528,6 +528,9 @@ export async function inferSchema( | |||
{ name: "database", typ: { resource: "postgresql" } }, | |||
...inferedSchema.args, | |||
]; | |||
} else if (language === "duckdb") { | |||
const { parse_sql } = await import("./wasm/regex/windmill_parser_wasm.js"); |
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.
This file needs to be updated and checked in into version control (If parser has changed)
AFAIK running cli/build.sh should be enough
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.
Important
Looks good to me! 👍
Reviewed ab52287 in 1 minute and 48 seconds. Click for details.
- Reviewed
53
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. cli/build.sh:2
- Draft comment:
Consider auto‐detecting the OS to use the proper sed (e.g. fallback to gsed on macOS) or add a more detailed instruction on GNU sed installation. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. cli/sync.ts:347
- Draft comment:
DuckDB support is added via mapping to 'duckdb.sql'. Ensure that this new branch is consistent with other SQL language handling and that tests cover duckdb-specific cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure consistency and test coverage, which violates the rule against asking the author to ensure things are tested or consistent. It does not provide a specific suggestion or point out a specific issue.
3. cli/wasm/regex/windmill_parser_wasm.js:240
- Draft comment:
If DuckDB queries have syntax that differs from standard SQL, consider adding a dedicated parser (e.g. parse_duckdb) in the WASM module. Otherwise, confirm that parse_sql is sufficient. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. cli/build.sh:2
- Draft comment:
Typographical suggestion: Consider using "macOS" instead of "mac OS" for consistency with Apple's branding. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is an extremely minor stylistic change that doesn't affect functionality. While technically correct about Apple's branding, it's not a meaningful code improvement. The comment is perfectly understandable either way. We should focus PR comments on substantive issues. The comment is factually correct about Apple's official branding, and consistency in documentation can be important. While technically correct, this level of nitpicking about documentation styling is too minor to warrant a PR comment and could be seen as unnecessarily pedantic. Delete this comment as it's too minor and doesn't affect code functionality or clarity.
Workflow ID: wflow_ENVuMnUUHc4n14xs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
||
fn string_to_duckdb_date(s: &str) -> Result<duckdb::types::Value> { | ||
use chrono::Datelike; | ||
let date = chrono::NaiveDate::parse_from_str(s, "%Y-%m-%d").unwrap(); |
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.
The unwrap()
call on parse_from_str
will cause a panic if the date string is invalid. Consider using proper error handling with map_err
similar to the pattern used in other parsing functions to return a meaningful error instead of crashing.
let date = chrono::NaiveDate::parse_from_str(s, "%Y-%m-%d").unwrap(); | |
let date = chrono::NaiveDate::parse_from_str(s, "%Y-%m-%d") | |
.map_err(|e| anyhow::anyhow!("Invalid date format: {}", e))?; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
let idx = query.rfind("-- $").unwrap_or(query.len()); | ||
// find next \n starting from idx and return everything after it | ||
let idx = query[idx..].find('\n').map(|i| i + idx).unwrap_or(0); | ||
&query[idx..] |
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.
The signature truncation logic has a potential issue. When rfind
doesn't find "-- $" and returns query.len()
, query[idx..]
will be an empty slice. Then trying to find '\n'
in this empty slice will always return None
, resulting in idx
being set to 0, which returns the entire query instead of skipping the signature.
Consider handling the case where no signature is found by either:
- Returning the original query directly when
idx == query.len()
- Using a different default value than 0 when no newline is found
This would ensure correct behavior in all cases, including when there's no signature pattern in the query.
let idx = query.rfind("-- $").unwrap_or(query.len()); | |
// find next \n starting from idx and return everything after it | |
let idx = query[idx..].find('\n').map(|i| i + idx).unwrap_or(0); | |
&query[idx..] | |
let idx = match query.rfind("-- $") { | |
Some(pos) => { | |
// Find the next newline after the signature marker | |
let after_marker = &query[pos..]; | |
match after_marker.find('\n') { | |
Some(i) => pos + i + 1, // +1 to skip the newline | |
None => query.len(), // No newline after marker, return empty string | |
} | |
}, | |
None => 0, // No signature marker found, return the entire query | |
}; | |
&query[idx..] | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
This is the intended behavior
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.
Caution
Changes requested ❌
Reviewed dbb3e2d in 1 minute and 32 seconds. Click for details.
- Reviewed
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_eOeg2lX3O00wROpe
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
"double" | "float8" => duckdb::types::Value::Double(v), | ||
"decimal" | "numeric" => { | ||
duckdb::types::Value::Decimal(Decimal::from_f64(v).ok_or_else(|| { | ||
Error::ExecutionErr("Could not convert f64 to Decimal".to_string()) |
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.
Consider including the f64
value in the error message for easier debugging.
Error::ExecutionErr("Could not convert f64 to Decimal".to_string()) | |
Error::ExecutionErr(format!("Could not convert f64 value {} to Decimal", v)) |
"float" | "float4" | "real" => duckdb::types::Value::Float(v as f32), | ||
"double" | "float8" => duckdb::types::Value::Double(v), | ||
"decimal" | "numeric" => { | ||
duckdb::types::Value::Decimal(Decimal::from_f64(v).ok_or_else(|| { |
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.
The conversion from f64
to Decimal
using unwrap()
could cause a panic if the conversion fails. Consider using proper error handling:
duckdb::types::Value::Decimal(
Decimal::from_f64(v).ok_or_else(||
Error::ExecutionErr("Could not convert f64 to Decimal".to_string())
)?
)
This approach propagates the error instead of crashing the application.
duckdb::types::Value::Decimal(Decimal::from_f64(v).ok_or_else(|| { | |
duckdb::types::Value::Decimal( | |
Decimal::from_f64(v).ok_or_else(|| | |
Error::ExecutionErr("Could not convert f64 to Decimal".to_string()) | |
)? | |
) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Important
This pull request adds comprehensive support for DuckDB SQL language across the backend, frontend, and CLI, enabling parsing, execution, and script handling for DuckDB.
windmill-parser-sql
withparse_duckdb_sig()
andparse_duckdb_file()
functions.windmill-common
to handle DuckDB connection settings and execution induckdb_executor.rs
.script_helpers.ts
.Editor.svelte
,HighlightCode.svelte
, andLanguageIcon.svelte
.consts.ts
with DuckDB types.infer.ts
andinferArgSig.ts
to handle DuckDB argument signatures.metadata.ts
andscript.ts
to recognize DuckDB scripts.wasm/regex/windmill_parser_wasm.js
..gitignore
to ignore DuckDB files.Cargo.toml
to include DuckDB dependencies.This description was created by
for dbb3e2d. You can customize this summary. It will automatically update as commits are pushed.