This repository was archived by the owner on May 7, 2026. It is now read-only.
feat: add wren-core-wasm module with browser WASM support - #1568
Merged
Conversation
github-actions
Bot
added
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new wren-core-wasm WebAssembly package and TypeScript SDK (with examples, tests, build scripts, and npm packaging), CI and publish workflows, DataFusion v53 upgrades and related core/MDL/dialect changes, Python bindings and a DataFusion connector with tests, release automation updates, and a deprecation note for ibis-server. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant SDK
participant WASM
participant DataFusion
participant MDL
Browser->>SDK: WrenEngine.init(wasmUrl?)
SDK->>WASM: init(module_or_path)
WASM->>DataFusion: create SessionContext (single-thread)
Browser->>SDK: registerJson(name, data)
SDK->>WASM: register_parquet/register_json
WASM->>DataFusion: infer schema -> create MemTable -> register
Browser->>SDK: loadMDL(manifest, source)
SDK->>WASM: load_mdl(mdl_json, source)
alt source is http(s)
WASM->>DataFusion: register ListingTable(s) for URL mode
else source == ""
WASM->>DataFusion: analyze with previously registered tables (fallback)
else local mode
WASM->>DataFusion: lookup catalog/schema tables
end
WASM->>MDL: analyze manifest -> apply semantic transforms
Browser->>SDK: query(sql)
SDK->>WASM: query(sql)
WASM->>DataFusion: ctx.sql(sql) -> collect batches
DataFusion-->>WASM: Arrow record batches
WASM-->>SDK: JSON result string
SDK-->>Browser: parsed JSON records
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wren-core/core/src/mdl/function/dialect/bigquery/mod.rs (1)
122-125:⚠️ Potential issue | 🟡 MinorDuplicate function registrations:
json_remove()andjson_set()appear twice.Lines 122-125 register the same functions that appear again later in the list:
json_remove()is registered on both line 122 and line 124json_set()is registered on both line 123 and line 125This appears to be a copy-paste error.
🔧 Proposed fix: Remove duplicate registrations
json_query(), json_query_array(), json_remove(), json_set(), - json_remove(), - json_set(), json_strip_nulls(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core/core/src/mdl/function/dialect/bigquery/mod.rs` around lines 122 - 125, Duplicate registrations for the JSON functions were introduced: remove the extra entries so each function is registered only once — delete one of the repeated json_remove() and one of the repeated json_set() entries in the BigQuery dialect registration list (look for occurrences of json_remove and json_set in mod.rs) leaving a single registration of each, then run the build/tests to confirm no duplicate-symbol or registration regressions.ibis-server/pyproject.toml (1)
2-7:⚠️ Potential issue | 🟠 MajorResolve the package-namespace collision between
wren-engine-serverandwren-engine.The wheel includes
packages = [{ include = "wren" }](line 7), and the new runtime dependencywren-engine(line 60) also publishes a top-levelwrenpackage. Both distributions cannot coexist in the same environment—imports will resolve unpredictably based on install order. Either remove the localwrenpackage from ibis-server if it is now provided by the dependency, or removewren-enginefrom the dependency list if the local version is the canonical one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ibis-server/pyproject.toml` around lines 2 - 7, The pyproject declares a local top-level package via packages = [{ include = "wren" }] while also adding the runtime dependency "wren-engine", causing a package-namespace collision; decide which source should own the top-level wren package and update accordingly: either remove the local packages entry (packages = [{ include = "wren" }]) from pyproject.toml so imports come from the "wren-engine" dependency, or remove "wren-engine" from the dependencies list so the project's local wren package remains canonical; update pyproject.toml to reflect that single source of truth and run a local build/install to verify no import conflicts.wren-core-py/src/context.rs (1)
98-117:⚠️ Potential issue | 🟠 MajorPreserve
propertiesin the no-MDL constructor path.This branch always stores
self.propertiesas{}. If the caller creates an empty context, registers tables, and then callsload_mdl(), RLAC/CLAC analysis will run with empty session properties and there’s no later way to recover the values passed intonew(...).💡 Suggested direction
- let Some(mdl_base64) = mdl_base64 else { + let properties_ref = Python::attach(|py: Python<'_>| { + // reuse the existing tuple/frozenset parsing here + // and return Arc<HashMap<String, Option<String>>> + })?; + + let Some(mdl_base64) = mdl_base64 else { let data_source = data_source .map(|ds| DataSource::from_str(ds).map_err(CoreError::from)) .transpose()?; ... return Ok(Self { base_ctx: ctx.clone(), ctx: ctx.clone(), exec_ctx: ctx, mdl: Arc::new(AnalyzedWrenMDL::default()), - properties: Arc::new(HashMap::new()), + properties: Arc::clone(&properties_ref), runtime: Arc::new(runtime), }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-py/src/context.rs` around lines 98 - 117, In the mdl_base64 None branch, preserve the session properties passed into the constructor instead of always using an empty map: locate the branch that builds and returns Self (the block that calls wren_core::mdl::create_wren_ctx(...) and Self::register_function_by_data_source(...)), and replace the hard-coded Arc::new(HashMap::new()) used for properties with the properties value provided to the constructor (e.g., Arc::new(properties) or Arc::new(properties.clone()) as appropriate), ensuring the returned Self uses the original properties so later load_mdl()/RLAC/CLAC analysis sees the intended session properties.
🧹 Nitpick comments (4)
wren-core/core/src/mdl/function/remote_function.rs (1)
223-247: Add a regression test for case-sensitive alias registration.Line 223 changes the aliasing rule, but the remaining test coverage only exercises
From<RemoteFunction>. Please add a small unit test aroundnew_with_original_name("toYear", "toyear", ...)to pinname(),original_name(), andaliases()behavior.Suggested test
+ #[test] + fn test_new_with_original_name_keeps_original_and_parse_alias() { + let udf = ByPassScalarUDF::new_with_original_name( + "toYear", + "toyear", + DataType::Int64, + ); + + assert_eq!(udf.name(), "toYear"); + assert_eq!(udf.original_name(), Some("toYear")); + assert_eq!(udf.aliases(), &vec!["toyear".to_string()]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core/core/src/mdl/function/remote_function.rs` around lines 223 - 247, Add a unit test that constructs a RemoteFunction via RemoteFunction::new_with_original_name("toYear", "toyear", <a DataType>), then assert RemoteFunction::name() equals "toYear", RemoteFunction::original_name() is Some("toYear"), and RemoteFunction::aliases() contains "toyear" (and does not erroneously drop or duplicate the alias); place the test alongside existing RemoteFunction tests and use the same DataType used elsewhere (or a simple concrete DataType) to keep it focused on name/alias behavior.wren-core-py/tests/test_modeling_core.py (1)
136-152: Avoid pinning the full DataFusion function count.These assertions already changed once for the DataFusion bump, so they’ll keep breaking on harmless upstream catalog churn. The test would be more stable if it checked that the custom CSV functions were loaded and that the CSV-backed list is larger than the baseline.
More stable assertion shape
def test_read_function_list(): path = "tests/functions.csv" session_context = SessionContext(manifest_str, path) - functions = session_context.get_available_functions() - assert len(functions) == 290 + functions_with_csv = session_context.get_available_functions() + assert any(f.name == "add_custom" for f in functions_with_csv) rewritten_sql = session_context.transform_sql( "SELECT add_two(c_custkey, c_custkey) FROM my_catalog.my_schema.customer" ) assert ( rewritten_sql == 'SELECT add_two(customer.c_custkey, customer.c_custkey) FROM (SELECT customer.c_custkey FROM (SELECT __source.c_custkey AS c_custkey FROM "main".customer AS __source) AS customer) AS customer' ) session_context = SessionContext(manifest_str, None) - functions = session_context.get_available_functions() - assert len(functions) == 283 + functions_without_csv = session_context.get_available_functions() + assert len(functions_with_csv) > len(functions_without_csv)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-py/tests/test_modeling_core.py` around lines 136 - 152, The test_read_function_list test pins exact counts which are brittle; update it to assert that SessionContext.get_available_functions() when created with path (using manifest_str and path) returns a larger set than the baseline SessionContext(manifest_str, None) and that at least one known CSV-provided function (e.g., a function name present in tests/functions.csv) appears in the functions list. Specifically, replace the fixed asserts with: call SessionContext(manifest_str, path) and SessionContext(manifest_str, None), capture their get_available_functions() results via the functions variable, assert len(functions_with_csv) > len(functions_baseline), and assert the presence of one or two expected CSV function identifiers; reference test_read_function_list, SessionContext, get_available_functions, manifest_str, and path to locate the change.wren-core-wasm/examples/url-mode.html (1)
22-29: Add a brief note about current URL-mode file layout limits.The setup text should mention that URL mode currently expects flat
{source}/{bare_name}.parquetnaming, so same bare table names across schemas can collide.Based on learnings: In
wren-core-wasm/src/lib.rs,load_mdl_url_modecurrently assumes flat URL layout and has known same-bare-name collision across schemas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-wasm/examples/url-mode.html` around lines 22 - 29, Update the setup paragraph to explicitly note that URL mode expects a flat {source}/{bare_name}.parquet layout and therefore identical bare table names across different schemas can collide; mention the limitation and that it stems from the current implementation in load_mdl_url_mode (and the higher-level loadMDL behavior) so users should ensure unique bare names or avoid same-named tables across schemas..github/workflows/publish-wren-core-wasm.yml (1)
63-66: Usenpm cifor release publish reproducibility.For publish workflows, deterministic installs are important. Prefer
npm ci(with lockfile) overnpm install.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-wren-core-wasm.yml around lines 63 - 66, Replace the "Install npm dependencies" workflow step's command in the publish job to use a deterministic install: change the run from "npm install" to "npm ci" (the step labeled "Install npm dependencies" in the job that runs in working-directory "wren-core-wasm"); ensure the repository includes a lockfile (package-lock.json) in that directory so npm ci can use it for reproducible installs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RELEASING.md`:
- Line 13: The release notes list now includes a new component `wren-core-wasm`
but the "Publish happens automatically" section omits it; update RELEASING.md to
add `wren-core-wasm` to the automatic publish bullet(s) so the release flow
covers that package (search for the table row containing `wren-core-wasm` and
the corresponding "Publish happens automatically" bullets and add the package
name and any specific publish trigger/step used by other npm entries).
In `@wren-core-wasm/examples/inline.html`:
- Around line 48-53: The renderTable function (and the other places rendering
query/error output around lines 68-70) injects unescaped dynamic content into
innerHTML; change these to safely escape or set textContent instead of building
HTML strings: in renderTable, build the table using DOM APIs
(document.createElement('table'/'tr'/'th'/'td') and assign cell.textContent =
keys/values) or explicitly HTML-escape keys and values before concatenation;
likewise replace any direct innerHTML assignments for query/error text with
element.textContent or sanitized content to prevent script injection.
In `@wren-core-wasm/examples/serve.mjs`:
- Around line 50-69: Validate the parsed range values and attach error handlers
to the read stream before piping: in the range handling branch (where
rangeHeader is parsed and start/end are computed) check that start < size and
end >= start and clamp end to size-1; if the range is invalid respond with 416
and a proper Content-Range header (`bytes */${size}`) instead of proceeding;
when creating the stream via createReadStream(filePath, { start, end }) register
an 'error' listener that logs the error and ends the response with an
appropriate status (and a 'close' or 'finish' handler to cleanup), and ensure
you compute Content-Length as (end - start + 1) only after validation so headers
match the stream.
In `@wren-core-wasm/examples/test-cdn.html`:
- Around line 52-57: renderTable currently builds an HTML string from row values
and is later inserted with innerHTML, opening an XSS/injection path; instead,
stop emitting unescaped HTML: change renderTable to create DOM nodes (table, tr,
th, td) programmatically or ensure every cell value is escaped before
concatenation, and when inserting results replace uses of element.innerHTML =
renderTable(rows) with appending the created nodes or setting textContent on td
elements; specifically update the renderTable function and the code that assigns
its output so that cell values use safe text nodes (or a proper escape function)
rather than raw string interpolation.
In `@wren-core-wasm/examples/url-mode.html`:
- Around line 57-63: The renderTable function and the places where innerHTML is
used render unescaped data directly into HTML, enabling XSS; fix by escaping
HTML-special characters for any dynamic cell or error text (implement a small
escapeHtml utility that replaces & < > " ' ` with entities) and use it when
building table cells in renderTable (replace `${r[k] ?? ''}` with escaped value)
or, better, construct DOM nodes and set textContent for cell values; likewise
sanitize the error text before assigning to innerHTML or switch to textContent
when showing errors.
In `@wren-core-wasm/README.md`:
- Around line 179-181: Add a short note to the README documenting the known
bare-name collision in URL mode: explain that load_mdl_url_mode assumes a flat
URL layout ({source}/{bare_name}.parquet) so MDL tableReference entries that
share the same bare table name across different schemas (e.g., "raw"."orders" vs
"staging"."orders") will both resolve to the same .../orders.parquet and
silently collide; mention this limitation and recommend either using unique bare
names, a namespaced URL layout, or avoiding URL mode for models that would
collide.
In `@wren-core-wasm/scripts/build.mjs`:
- Around line 42-48: The build script currently skips missing pkg artifacts
silently; modify the copy/validation logic around the pkgFiles loop so it fails
fast if any required file from pkgFiles is missing: for each file in pkgFiles
(used in the for (const file of pkgFiles) loop, with src = resolve(pkg, file)),
check existsSync(src) and if not present throw or process.exit(1) with a
descriptive error mentioning the missing file and source path; likewise, extend
the later validation (which currently only checks the .wasm) to assert existence
of every entry in pkgFiles (and fail fast) rather than only validating the .wasm
so the dist/ build cannot be produced in a broken state.
In `@wren-core-wasm/sdk/src/wren_core_wasm.d.ts`:
- Around line 1-29: The hand-maintained declarations diverge from wasm-pack
output: update the exported types to match generated bindings by replacing or
regenerating this file with the wasm-pack (--target web) .d.ts output so that
init has the standard signature (accepting RequestInfo | BufferSource |
WebAssembly.Module and returning Promise<any>) and WrenEngine methods match the
generated types; alternatively adjust the declaration for init to return
Promise<any> and simplify its parameter union to the wasm-pack form, and add a
CI check to diff the checked-in .d.ts against the build output to prevent future
drift (refer to the init function and WrenEngine class when locating changes).
In `@wren-core-wasm/src/lib.rs`:
- Around line 450-453: The JSON writer is configured to drop null fields via
WriterBuilder::with_explicit_nulls(false), which removes nullable columns from
output; change this to preserve SQL NULLs by enabling explicit nulls (call
with_explicit_nulls(true) or remove the override so explicit nulls are emitted)
in the WriterBuilder used with build::<_, JsonArray>(&mut buf) so rows like {
amount: null } serialize as { "amount": null } consistently.
In `@wren-core/core/src/logical_plan/optimize/simplify_timestamp.rs`:
- Line 115: The field type for `simplifier` is missing the generic parameter;
change its type from `&'a ExprSimplifier` to `&'a
ExprSimplifier<SimplifyContext<'a>>` so it matches DataFusion v53's generic
`ExprSimplifier<S>`; update the surrounding struct/impl generics/signatures that
reference `simplifier` (e.g., the type parameter list where `simplifier` is
declared and any methods/impl blocks using that field) to include the
`SimplifyContext<'a>` type argument as needed so the code compiles.
In `@wren-core/core/src/mdl/mod.rs`:
- Around line 3989-4028: The test writes Parquet fixtures into a fixed temp dir
which can collide across parallel runs; replace the manual dir creation using
std::env::temp_dir() and std::fs::create_dir_all(&dir) with a unique
tempfile::TempDir (e.g., let tmp = tempfile::tempdir()?; let parquet_path =
tmp.path().join("data.parquet")) and use tmp.path() for all file paths so the
directory is unique and automatically cleaned up, updating the url construction
that uses parquet_path.display() accordingly; apply the same change to the other
test function test_analyze_with_url_tables_local_file_datasource so both
fixtures use tempfile::TempDir.
---
Outside diff comments:
In `@ibis-server/pyproject.toml`:
- Around line 2-7: The pyproject declares a local top-level package via packages
= [{ include = "wren" }] while also adding the runtime dependency "wren-engine",
causing a package-namespace collision; decide which source should own the
top-level wren package and update accordingly: either remove the local packages
entry (packages = [{ include = "wren" }]) from pyproject.toml so imports come
from the "wren-engine" dependency, or remove "wren-engine" from the dependencies
list so the project's local wren package remains canonical; update
pyproject.toml to reflect that single source of truth and run a local
build/install to verify no import conflicts.
In `@wren-core-py/src/context.rs`:
- Around line 98-117: In the mdl_base64 None branch, preserve the session
properties passed into the constructor instead of always using an empty map:
locate the branch that builds and returns Self (the block that calls
wren_core::mdl::create_wren_ctx(...) and
Self::register_function_by_data_source(...)), and replace the hard-coded
Arc::new(HashMap::new()) used for properties with the properties value provided
to the constructor (e.g., Arc::new(properties) or Arc::new(properties.clone())
as appropriate), ensuring the returned Self uses the original properties so
later load_mdl()/RLAC/CLAC analysis sees the intended session properties.
In `@wren-core/core/src/mdl/function/dialect/bigquery/mod.rs`:
- Around line 122-125: Duplicate registrations for the JSON functions were
introduced: remove the extra entries so each function is registered only once —
delete one of the repeated json_remove() and one of the repeated json_set()
entries in the BigQuery dialect registration list (look for occurrences of
json_remove and json_set in mod.rs) leaving a single registration of each, then
run the build/tests to confirm no duplicate-symbol or registration regressions.
---
Nitpick comments:
In @.github/workflows/publish-wren-core-wasm.yml:
- Around line 63-66: Replace the "Install npm dependencies" workflow step's
command in the publish job to use a deterministic install: change the run from
"npm install" to "npm ci" (the step labeled "Install npm dependencies" in the
job that runs in working-directory "wren-core-wasm"); ensure the repository
includes a lockfile (package-lock.json) in that directory so npm ci can use it
for reproducible installs.
In `@wren-core-py/tests/test_modeling_core.py`:
- Around line 136-152: The test_read_function_list test pins exact counts which
are brittle; update it to assert that SessionContext.get_available_functions()
when created with path (using manifest_str and path) returns a larger set than
the baseline SessionContext(manifest_str, None) and that at least one known
CSV-provided function (e.g., a function name present in tests/functions.csv)
appears in the functions list. Specifically, replace the fixed asserts with:
call SessionContext(manifest_str, path) and SessionContext(manifest_str, None),
capture their get_available_functions() results via the functions variable,
assert len(functions_with_csv) > len(functions_baseline), and assert the
presence of one or two expected CSV function identifiers; reference
test_read_function_list, SessionContext, get_available_functions, manifest_str,
and path to locate the change.
In `@wren-core-wasm/examples/url-mode.html`:
- Around line 22-29: Update the setup paragraph to explicitly note that URL mode
expects a flat {source}/{bare_name}.parquet layout and therefore identical bare
table names across different schemas can collide; mention the limitation and
that it stems from the current implementation in load_mdl_url_mode (and the
higher-level loadMDL behavior) so users should ensure unique bare names or avoid
same-named tables across schemas.
In `@wren-core/core/src/mdl/function/remote_function.rs`:
- Around line 223-247: Add a unit test that constructs a RemoteFunction via
RemoteFunction::new_with_original_name("toYear", "toyear", <a DataType>), then
assert RemoteFunction::name() equals "toYear", RemoteFunction::original_name()
is Some("toYear"), and RemoteFunction::aliases() contains "toyear" (and does not
erroneously drop or duplicate the alias); place the test alongside existing
RemoteFunction tests and use the same DataType used elsewhere (or a simple
concrete DataType) to keep it focused on name/alias behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0021e475-44df-4ce7-a55c-e173eec748f5
⛔ Files ignored due to path filters (5)
ibis-server/poetry.lockis excluded by!**/*.lockwren-core-py/Cargo.lockis excluded by!**/*.lockwren-core-wasm/Cargo.lockis excluded by!**/*.lockwren-core-wasm/examples/data/orders.parquetis excluded by!**/*.parquetwren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.github/workflows/publish-wren-core-wasm.yml.github/workflows/rc-release.yml.github/workflows/release-please.yml.github/workflows/wasm-ci.yml.release-please-manifest.jsonRELEASING.mdibis-server/README.mdibis-server/justfileibis-server/pyproject.tomlrelease-please-config.jsonwren-core-py/Cargo.tomlwren-core-py/src/context.rswren-core-py/src/extractor.rswren-core-py/tests/test_modeling_core.pywren-core-wasm/.claude/CLAUDE.mdwren-core-wasm/.gitignorewren-core-wasm/AGENT_GUIDE.mdwren-core-wasm/Cargo.tomlwren-core-wasm/LICENSEwren-core-wasm/README.mdwren-core-wasm/examples/inline.htmlwren-core-wasm/examples/serve.mjswren-core-wasm/examples/test-cdn.htmlwren-core-wasm/examples/url-mode.htmlwren-core-wasm/justfilewren-core-wasm/package.jsonwren-core-wasm/scripts/build.mjswren-core-wasm/sdk/src/index.tswren-core-wasm/sdk/src/wren_core_wasm.d.tswren-core-wasm/sdk/tests/index.test.mjswren-core-wasm/sdk/tsconfig.jsonwren-core-wasm/src/lib.rswren-core/Cargo.tomlwren-core/benchmarks/Cargo.tomlwren-core/core/Cargo.tomlwren-core/core/src/logical_plan/analyze/access_control.rswren-core/core/src/logical_plan/analyze/model_anlayze.rswren-core/core/src/logical_plan/optimize/simplify_timestamp.rswren-core/core/src/logical_plan/optimize/type_coercion.rswren-core/core/src/logical_plan/utils.rswren-core/core/src/mdl/context.rswren-core/core/src/mdl/dataset.rswren-core/core/src/mdl/dialect/inner_dialect.rswren-core/core/src/mdl/dialect/wren_dialect.rswren-core/core/src/mdl/function/dialect/bigquery/mod.rswren-core/core/src/mdl/function/dialect/bigquery/scalar.rswren-core/core/src/mdl/function/remote_function.rswren-core/core/src/mdl/function/scalar/mod.rswren-core/core/src/mdl/function/scalar/to_char.rswren-core/core/src/mdl/mod.rswren-core/core/src/mdl/utils.rswren-core/wren-example/Cargo.tomlwren/justfilewren/pyproject.tomlwren/src/wren/connector/datafusion.pywren/src/wren/connector/factory.pywren/src/wren/mdl/cte_rewriter.pywren/src/wren/model/__init__.pywren/src/wren/model/data_source.pywren/src/wren/model/field_registry.pywren/tests/conftest.pywren/tests/connectors/test_datafusion.py
💤 Files with no reviewable changes (2)
- ibis-server/justfile
- wren-core/core/src/mdl/dialect/wren_dialect.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wren-core-wasm/src/lib.rs (1)
228-243: Simplify redundant HashSet in URL mode.The
registered_originsHashSet is created at line 229 and immediately checked at line 231. Since the HashSet is always empty when created,insert()will always returntrue, making the conditional redundant. Unlikeload_mdl_fallback(line 342) where the HashSet accumulates across multiple model iterations, here there's only one origin from thesourceURL.♻️ Proposed simplification
if scheme == "http" || scheme == "https" { - let mut registered_origins: HashSet<String> = HashSet::new(); let origin = parsed_base.origin().unicode_serialization(); - if registered_origins.insert(origin.clone()) { - let http_store = object_store::http::HttpBuilder::new() - .with_url(&origin) - .build() - .map_err(|e| { - JsError::new(&format!("Failed to create HTTP store for {origin}: {e}")) - })?; - let store_url = url::Url::parse(&format!("{origin}/")) - .map_err(|e| JsError::new(&format!("Invalid base URL: {e}")))?; - self.ctx - .register_object_store(&store_url, Arc::new(http_store)); - } + let http_store = object_store::http::HttpBuilder::new() + .with_url(&origin) + .build() + .map_err(|e| { + JsError::new(&format!("Failed to create HTTP store for {origin}: {e}")) + })?; + let store_url = url::Url::parse(&format!("{origin}/")) + .map_err(|e| JsError::new(&format!("Invalid base URL: {e}")))?; + self.ctx + .register_object_store(&store_url, Arc::new(http_store)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-wasm/src/lib.rs` around lines 228 - 243, registered_origins is created and checked only locally so insert() will always be true; remove the redundant HashSet and simply use parsed_base.origin().unicode_serialization() (origin) to build the HttpBuilder and register the store: obtain origin, build http_store via object_store::http::HttpBuilder::new().with_url(&origin).build(), parse store_url via url::Url::parse(&format!("{origin}/")), then call self.ctx.register_object_store(&store_url, Arc::new(http_store)); keep the existing error mapping (JsError) around build/parse and remove the registered_origins variable and its insert-based conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wren-core-wasm/src/lib.rs`:
- Around line 228-243: registered_origins is created and checked only locally so
insert() will always be true; remove the redundant HashSet and simply use
parsed_base.origin().unicode_serialization() (origin) to build the HttpBuilder
and register the store: obtain origin, build http_store via
object_store::http::HttpBuilder::new().with_url(&origin).build(), parse
store_url via url::Url::parse(&format!("{origin}/")), then call
self.ctx.register_object_store(&store_url, Arc::new(http_store)); keep the
existing error mapping (JsError) around build/parse and remove the
registered_origins variable and its insert-based conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5b4018a-4b2b-4e68-9813-da9efe453d88
📒 Files selected for processing (2)
RELEASING.mdwren-core-wasm/src/lib.rs
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…er#1541) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… C-2 Phase 2) (Canner#1542) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…entifier (Canner#1557) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…anner#1559) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…flow (Canner#1562) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
table_reference() now returns Option<&str> after wren-core-base changes, causing WASM build failures. Use unwrap_or_default() to maintain the same behavior (empty string fallback). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ease docs
- Change with_explicit_nulls(false) to true so nullable columns
serialize as {"amount": null} instead of being silently dropped
- Add wren-core-wasm to RELEASING.md publish workflow list
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goldmedal
force-pushed
the
feat/wren-wasm
branch
from
April 16, 2026 13:49
3246c08 to
9a2c077
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-core/core/src/mdl/dialect/inner_dialect.rs (1)
42-94:⚠️ Potential issue | 🟠 MajorRestore a Snowflake-specific UNNEST/FLATTEN hook before removing the old one.
Removing the unnest/alias extension points from
InnerDialectleavesSnowflakeDialectwith onlyunnest_as_table_factor(). That is not enough to emit Snowflake’sTABLE(FLATTEN(...))form or the alias rewrite that used to go with it, so array-expansion queries will now fall back to the generic unparser and produce invalid Snowflake SQL.Also applies to: 421-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core/core/src/mdl/dialect/inner_dialect.rs` around lines 42 - 94, The InnerDialect trait removed the Snowflake-specific hooks that allowed dialects to emit TABLE(FLATTEN(...)) and perform the alias rewrite for UNNEST/FLATTEN; restore a dedicated hook (e.g., a method to rewrite an UNNEST expression into a table-factor AST node and an optional alias-rewrite hook) on InnerDialect so SnowflakeDialect can override it, implement the Snowflake logic in SnowflakeDialect to emit TABLE(FLATTEN(...)) and rewrite column aliases, and update the unparser call sites that handle UNNEST to consult the new InnerDialect methods (see unnest_as_table_factor(), col_alias_overrides(), and the SnowflakeDialect impl) so array-expansion queries produce valid Snowflake SQL.
🧹 Nitpick comments (1)
RELEASING.md (1)
66-66: Update line 66 to include npm RC releases in the documented artifacts.The RC workflow (
rc-release.yml) supports publishingwren-core-wasmwithnpm_tag: rc, but the RELEASING.md documentation at line 66 only mentions Docker and PyPI artifacts.Suggested update
- - Publishes the artifact (Docker image without `latest` tag, or PyPI with PEP 440 RC version like `0.25.0rc1`) + - Publishes the artifact (Docker image without `latest` tag, PyPI with PEP 440 RC version like `0.25.0rc1`, or npm with RC dist-tag)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RELEASING.md` at line 66, Update the RELEASING.md sentence that currently lists published artifacts to also mention npm RC releases; specifically add that the RC workflow (rc-release.yml) can publish the wren-core-wasm package to npm using npm_tag: rc, so the line that reads "Docker image without `latest` tag, or PyPI with PEP 440 RC version..." becomes inclusive of npm RC releases for wren-core-wasm via npm_tag: rc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren-core-wasm/src/lib.rs`:
- Around line 248-257: Currently register_listing_table(name,
&parquet_url).await? mutates self.ctx as each model is processed, leaving stale
tables if a later schema inference fails; instead, first stage schema inference
for every model in manifest.models (call the same logic used by
register_listing_table but stop short of mutating self.ctx and collect results),
e.g. build a vec of (name, parquet_url, inferred_schema) by invoking the
inference portion for each model, and only after all inferences succeed iterate
that vec to call self.register_listing_table or the ctx-mutating registration
step for each entry; target symbols: manifest.models, register_listing_table,
self.ctx, loadMDL.
- Around line 49-53: The WrenEngine::new() constructor currently builds a
DataFusion SessionConfig with with_target_partitions(1) but does not set a
default timezone; update new() to set the DataFusion session timezone to
"+00:00" (UTC) when creating the SessionConfig (or immediately set it on the
SessionContext returned by SessionContext::new_with_config) so the WASM session
timezone matches create_wren_ctx() normalization; modify the SessionConfig
creation line that uses datafusion::execution::context::SessionConfig::new() and
ensure the resulting SessionContext is created with the UTC timezone.
In `@wren-core/core/src/logical_plan/optimize/type_coercion.rs`:
- Around line 822-833: The current scalar UDF coercion builds synthetic fields
from current_types which loses nullability/nested metadata; instead construct
the argument fields from each expression using e.to_field(schema) (like the
aggregate UDF path does) before calling fields_with_udf so signature matching
sees the original Field metadata; update the block that creates current_fields
and new_types (the use of current_types, current_fields, fields_with_udf, and
the expressions.cast_to call) to derive fields from expressions via
to_field(schema) and then proceed with fields_with_udf and
cast_to(new_types[i].data_type(), schema).
- Around line 617-618: The match currently treats Expr::SetComparison as a leaf;
instead add a dedicated match arm for Expr::SetComparison that mirrors the
InSubquery handling: call analyze_internal() on the subquery expression,
determine the subquery result type, then perform type coercion between the left
expression and the subquery type (using the same coercion helper used for
InSubquery) and return the appropriately wrapped Transformed result; reference
Expr::SetComparison, analyze_internal(), the InSubquery match-arm logic, and the
type-coercion helper so you implement the same sequence (analyze subquery ->
compute target type -> coerce left expr) rather than returning
Transformed::no(expr).
In `@wren-core/core/src/mdl/mod.rs`:
- Around line 491-495: The function transform_sql is currently behind
#[cfg(feature = "multi-thread")] but test_sync_transform calls it
unconditionally, causing builds without the feature to fail; make transform_sql
available regardless of the "multi-thread" feature by removing the #[cfg(feature
= "multi-thread")] gate (or replace it with a no-op/build-compat shim) so the
symbol transform_sql always exists and delegates to the async
transform_sql_with_ctx (or provides a clear fallback) so test_sync_transform
compiles in non-multi-thread builds.
---
Outside diff comments:
In `@wren-core/core/src/mdl/dialect/inner_dialect.rs`:
- Around line 42-94: The InnerDialect trait removed the Snowflake-specific hooks
that allowed dialects to emit TABLE(FLATTEN(...)) and perform the alias rewrite
for UNNEST/FLATTEN; restore a dedicated hook (e.g., a method to rewrite an
UNNEST expression into a table-factor AST node and an optional alias-rewrite
hook) on InnerDialect so SnowflakeDialect can override it, implement the
Snowflake logic in SnowflakeDialect to emit TABLE(FLATTEN(...)) and rewrite
column aliases, and update the unparser call sites that handle UNNEST to consult
the new InnerDialect methods (see unnest_as_table_factor(),
col_alias_overrides(), and the SnowflakeDialect impl) so array-expansion queries
produce valid Snowflake SQL.
---
Nitpick comments:
In `@RELEASING.md`:
- Line 66: Update the RELEASING.md sentence that currently lists published
artifacts to also mention npm RC releases; specifically add that the RC workflow
(rc-release.yml) can publish the wren-core-wasm package to npm using npm_tag:
rc, so the line that reads "Docker image without `latest` tag, or PyPI with PEP
440 RC version..." becomes inclusive of npm RC releases for wren-core-wasm via
npm_tag: rc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e6adea1-fced-47b9-8634-9b4f176a5df2
⛔ Files ignored due to path filters (5)
ibis-server/poetry.lockis excluded by!**/*.lockwren-core-py/Cargo.lockis excluded by!**/*.lockwren-core-wasm/Cargo.lockis excluded by!**/*.lockwren-core-wasm/examples/data/orders.parquetis excluded by!**/*.parquetwren/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.github/workflows/publish-wren-core-wasm.yml.github/workflows/rc-release.yml.github/workflows/release-please.yml.github/workflows/wasm-ci.yml.release-please-manifest.jsonRELEASING.mdibis-server/README.mdibis-server/justfileibis-server/pyproject.tomlrelease-please-config.jsonwren-core-py/Cargo.tomlwren-core-py/src/context.rswren-core-py/src/extractor.rswren-core-py/tests/test_modeling_core.pywren-core-wasm/.claude/CLAUDE.mdwren-core-wasm/.gitignorewren-core-wasm/AGENT_GUIDE.mdwren-core-wasm/Cargo.tomlwren-core-wasm/LICENSEwren-core-wasm/README.mdwren-core-wasm/examples/inline.htmlwren-core-wasm/examples/serve.mjswren-core-wasm/examples/test-cdn.htmlwren-core-wasm/examples/url-mode.htmlwren-core-wasm/justfilewren-core-wasm/package.jsonwren-core-wasm/scripts/build.mjswren-core-wasm/sdk/src/index.tswren-core-wasm/sdk/src/wren_core_wasm.d.tswren-core-wasm/sdk/tests/index.test.mjswren-core-wasm/sdk/tsconfig.jsonwren-core-wasm/src/lib.rswren-core/Cargo.tomlwren-core/benchmarks/Cargo.tomlwren-core/core/Cargo.tomlwren-core/core/src/logical_plan/analyze/access_control.rswren-core/core/src/logical_plan/analyze/model_anlayze.rswren-core/core/src/logical_plan/optimize/simplify_timestamp.rswren-core/core/src/logical_plan/optimize/type_coercion.rswren-core/core/src/logical_plan/utils.rswren-core/core/src/mdl/context.rswren-core/core/src/mdl/dataset.rswren-core/core/src/mdl/dialect/inner_dialect.rswren-core/core/src/mdl/dialect/wren_dialect.rswren-core/core/src/mdl/function/dialect/bigquery/mod.rswren-core/core/src/mdl/function/dialect/bigquery/scalar.rswren-core/core/src/mdl/function/remote_function.rswren-core/core/src/mdl/function/scalar/mod.rswren-core/core/src/mdl/function/scalar/to_char.rswren-core/core/src/mdl/mod.rswren-core/core/src/mdl/utils.rswren-core/wren-example/Cargo.tomlwren/justfilewren/pyproject.tomlwren/src/wren/connector/datafusion.pywren/src/wren/connector/factory.pywren/src/wren/mdl/cte_rewriter.pywren/src/wren/model/__init__.pywren/src/wren/model/data_source.pywren/src/wren/model/field_registry.pywren/tests/conftest.pywren/tests/connectors/test_datafusion.py
💤 Files with no reviewable changes (2)
- ibis-server/justfile
- wren-core/core/src/mdl/dialect/wren_dialect.rs
✅ Files skipped from review due to trivial changes (27)
- wren-core-wasm/.gitignore
- ibis-server/README.md
- wren-core-py/Cargo.toml
- wren-core-wasm/LICENSE
- wren/tests/conftest.py
- wren-core/core/src/logical_plan/analyze/access_control.rs
- wren-core/benchmarks/Cargo.toml
- wren/justfile
- release-please-config.json
- wren-core/wren-example/Cargo.toml
- wren-core/core/src/logical_plan/optimize/simplify_timestamp.rs
- wren-core/core/src/mdl/function/scalar/to_char.rs
- wren-core/core/src/mdl/dataset.rs
- wren-core-wasm/sdk/tsconfig.json
- .release-please-manifest.json
- wren-core-wasm/examples/test-cdn.html
- wren-core/core/src/mdl/function/dialect/bigquery/scalar.rs
- wren-core-wasm/.claude/CLAUDE.md
- .github/workflows/publish-wren-core-wasm.yml
- wren-core-wasm/examples/url-mode.html
- wren-core-wasm/README.md
- wren-core-wasm/package.json
- wren-core-wasm/scripts/build.mjs
- wren-core-wasm/Cargo.toml
- wren-core-wasm/justfile
- wren-core-wasm/AGENT_GUIDE.md
- wren-core-wasm/sdk/src/wren_core_wasm.d.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- wren/src/wren/model/field_registry.py
- wren-core/core/src/mdl/context.rs
- ibis-server/pyproject.toml
- wren-core/core/src/logical_plan/utils.rs
- wren-core/core/src/mdl/utils.rs
- wren/src/wren/connector/factory.py
- wren/src/wren/mdl/cte_rewriter.py
- wren-core-py/tests/test_modeling_core.py
- wren-core/core/src/mdl/function/dialect/bigquery/mod.rs
- wren-core-wasm/examples/serve.mjs
- .github/workflows/wasm-ci.yml
- wren-core/core/Cargo.toml
- .github/workflows/rc-release.yml
- .github/workflows/release-please.yml
- wren-core-wasm/sdk/src/index.ts
- wren-core-wasm: default session time zone to UTC so browser timestamp semantics match the native SessionContext. - wren-core-wasm: stage URL-mode table inferences before mutating self.ctx so a failed loadMDL does not leave partially-registered tables behind. - wren-core: port SetComparison (ANY/ALL subquery) type coercion from upstream DataFusion v53 so the left expression and subquery result type are aligned like InSubquery. - wren-core: use e.to_field(schema) for scalar UDF argument coercion to preserve nullability and nested metadata, matching the aggregate UDF path. - wren-core: gate test_sync_transform on the multi-thread feature so cargo test --no-default-features compiles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-core/core/src/logical_plan/optimize/type_coercion.rs (1)
603-638:⚠️ Potential issue | 🟠 MajorPreserve
filteranddistinctfields inWindowFunctionParamsduring type coercion.The destructuring at lines 607–614 drops
filteranddistinctwith the..wildcard, and the rebuild (lines 632–637) omits the corresponding builder calls. In DataFusion v53,filter(the FILTER clause on window aggregates) anddistinctare semantic fields. Losing them silently changes query results.Proposed fix
let WindowFunction { fun, params: WindowFunctionParams { args, partition_by, order_by, window_frame, + filter, null_treatment, + distinct, .. }, } = *window_fun; let window_frame = coerce_window_frame(window_frame, self.schema, &order_by)?; @@ - Ok(Transformed::yes( - Expr::from(WindowFunction::new(fun, args)) + Ok(Transformed::yes(Expr::from( + WindowFunction::new(fun, args) .partition_by(partition_by) .order_by(order_by) .window_frame(window_frame) + .filter(filter) .null_treatment(null_treatment) + .distinct(distinct) - .build()?, - )) + .build()? + )))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core/core/src/logical_plan/optimize/type_coercion.rs` around lines 603 - 638, The destructuring of WindowFunctionParams drops the filter and distinct fields (using ..) and the rebuilt WindowFunction omits them, losing FILTER and DISTINCT semantics; update the pattern to explicitly bind filter and distinct from WindowFunctionParams (e.g., include filter, distinct in the let binding), and when reconstructing the Expr::from(WindowFunction::new(...)) chain, add the corresponding builder calls (.filter(filter) and .distinct(distinct)) before .build() so the original filter and distinct settings are preserved; reference WindowFunctionParams, WindowFunction::new, and the builder chain in type_coercion.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wren-core/core/src/logical_plan/optimize/type_coercion.rs`:
- Around line 603-638: The destructuring of WindowFunctionParams drops the
filter and distinct fields (using ..) and the rebuilt WindowFunction omits them,
losing FILTER and DISTINCT semantics; update the pattern to explicitly bind
filter and distinct from WindowFunctionParams (e.g., include filter, distinct in
the let binding), and when reconstructing the
Expr::from(WindowFunction::new(...)) chain, add the corresponding builder calls
(.filter(filter) and .distinct(distinct)) before .build() so the original filter
and distinct settings are preserved; reference WindowFunctionParams,
WindowFunction::new, and the builder chain in type_coercion.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aa150b5-17cf-4623-8e5d-24426cbb0cc0
📒 Files selected for processing (3)
wren-core-wasm/src/lib.rswren-core/core/src/logical_plan/optimize/type_coercion.rswren-core/core/src/mdl/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- wren-core/core/src/mdl/mod.rs
- wren-core-wasm/src/lib.rs
The `Expr::WindowFunction` arm of `TypeCoercionRewriter` was destructuring `WindowFunctionParams` with `..`, silently dropping `filter` and `distinct`. Reconstructing via `ExprFunctionExt` builder only replayed a subset of the fields, so `FILTER (WHERE ...)` and `DISTINCT` on window aggregates were lost whenever type coercion touched the expression. Rebuild the `WindowFunction` struct directly from all six fields, matching upstream DataFusion v53's approach, and drop the now-unused `ExprFunctionExt` import. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Re: the outside-diff finding in review pullrequestreview-4137001247 — fixed in 6da92fc. @coderabbitai the |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
douenergy
approved these changes
Apr 20, 2026
This was referenced Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wrenPython SDKwren-core-wasmmodule — a browser-ready WASM build of wren-core that supports inline MDL, URL-based table sources, and profile-based portable MDLwren-core-sdknpm package with TypeScript SDK wrapper, CI workflow, and release-please automationanalyze_with_url_tablesanddequote_identifierChanges
wren-core-wasm (new module)
src/lib.rs) exposing MDL analysis, SQL transformation, and query execution viawasm-bindgensdk/src/index.ts) wrapping the WASM module with ergonomic APIwasm-ci.yml) for build + test on PRspublish-wren-core-wasm.yml) triggered by release-pleasewren-core-wasmpackagewren-core
analyze_with_url_tablessupport for URL-based table sourceswren (Python SDK)
datafusionconnector for local Parquet/CSV analysisibis-server
Test plan
just testin wren-core-wasm/)wasm-ci.yml🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Deprecation
Documentation
Tests
Chores