chore: Merge back changes from upstream

[OlegWock]

Merge without squashing

This PR brings recent changes from upstream and reconciles git histories of upstream and fork. I marked all fork-specific changes with a comment

Resolves BLU-5815

Summary by CodeRabbit

Release Notes

• New Features

  • Added pluggable plan resolver system for custom query execution paths
  • Introduced ExternalDataset support for deferred data loading
  • Added SQL unparsing capabilities across multiple SQL dialects
  • Enhanced string type handling across Arrow Utf8, LargeUtf8, and Utf8View variants

• Dependencies

  • Upgraded DataFusion to 52.1.0 for improved performance and compatibility
  • Updated Tokio, PyO3, and related ecosystem crates to latest versions
  • Added tonic-prost for enhanced protobuf integration

• Improvements

  • Streamlined architecture documentation across all modules
  • Enhanced Python PyO3 interoperability with improved type safety
  • Added automated DataFusion proto file generation and management

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR introduces a plan resolver architecture to replace plan executors, enabling pluggable external table resolution strategies. Changes include: new PlanResolver trait in Rust with pipeline-based resolution; ExternalTableProvider and InlineTableProvider for deferring table execution; VegaFusionCodec for serializing extension nodes; MaterializedExportUpdate and MaterializedTaskValue for typed value materialization; DataFusion dependency upgraded to 52.1.0 with comprehensive datafusion_common.proto definitions; Python-side ExternalDataset and PlanResolver classes for user-defined resolution logic; proto code generation automation via download_datafusion_proto.py and gen_proto_py.py; and runtime refactoring to thread pipelines through task evaluation paths instead of direct context/executor references.

Sequence Diagram

sequenceDiagram
    participant User as User Code
    participant Runtime as VegaFusion Runtime
    participant ResolverPipeline as Resolver Pipeline
    participant UserResolver as User Resolver
    participant DataFusionResolver as DataFusion Resolver
    
    User->>Runtime: pre_transform_spec(spec, inline_datasets)
    Runtime->>Runtime: Apply pre-transforms
    Runtime->>ResolverPipeline: resolve(logical_plan)
    loop For each user resolver
        ResolverPipeline->>UserResolver: resolve_plan(plan)
        alt Plan returned
            UserResolver-->>ResolverPipeline: Plan(rewritten_plan)
            ResolverPipeline->>ResolverPipeline: Continue with next resolver
        else Table returned
            UserResolver-->>ResolverPipeline: Table(data)
            ResolverPipeline-->>Runtime: VegaFusionTable
        end
    end
    alt No table from user resolvers
        ResolverPipeline->>DataFusionResolver: resolve_plan(final_plan)
        DataFusionResolver->>DataFusionResolver: Execute via DataFusion
        DataFusionResolver-->>ResolverPipeline: VegaFusionTable
        ResolverPipeline-->>Runtime: VegaFusionTable
    end
    Runtime-->>User: Transformed spec + data

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

The changes are extensive and heterogeneous: new trait-based resolver system replacing executors, major proto schema additions (datafusion_common), proto code generation automation, Python FFI and bindings redesign, serialization codec implementation, materialization type refactoring across multiple crates, and async pipeline threading throughout task evaluation. Each area requires separate reasoning due to architectural significance.

Possibly related PRs

• Remove unusable public Python API #13 — Both PRs modify plan execution/resolution surface, replace PlanExecutor with PlanResolver, refactor Python executor paths, and update pre-transform logical plan APIs across runtime and Python modules.

Suggested reviewers

• mfranczel

[linear]

BLU-5815 Merge upstream changes into VegaFusion fork

[coderabbitai]

Actionable comments posted: 28

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

vegafusion-core/build.rs (1)

31-43: 🧹 Nitpick | 🔵 Trivial

De-duplicate the proto list before it drifts.

main() and gen_tonic() now carry the same proto paths by hand. The next proto addition can easily update one codegen path and silently miss the other.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vegafusion-core/build.rs` around lines 31 - 43, Extract the list of proto
paths into a single shared constant or accessor (e.g. a const PROTOS: &[&str] or
fn proto_files() -> Vec<&'static str>) and use that symbol from both main() and
gen_tonic() instead of repeating the hard-coded array; update the
tonic_prost_build::configure().compile_protos call to pass that shared PROTOS
and ensure any other code that previously duplicated the list references the
same symbol so additions/removals only happen in one place.

vegafusion-python/src/utils.rs (1)

96-104: ⚠️ Potential issue | 🟡 Minor

Change cast_bound::<PyAny> to cast_bound::<PyDict> for proper type validation.

When input is already Py<PyAny>, casting to PyAny always succeeds, making the "must be a string or dict" fallback unreachable. Non-dict inputs incorrectly proceed to depythonize and fail with a serde error instead of the advertised contract. Casting to PyDict properly validates that second-case inputs are dictionaries.

Proposed fix

-        } else if let Ok(chart_spec) = chart_spec.cast_bound::<PyAny>(py) {
+        } else if let Ok(chart_spec) = chart_spec.cast_bound::<PyDict>(py) {
             match depythonize::<ChartSpec>(&chart_spec.clone()) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vegafusion-python/src/utils.rs` around lines 96 - 104, The branch that tries
to handle a dict currently uses chart_spec.cast_bound::<PyAny>(py) which always
succeeds and makes the final "must be a string or dict" fallback unreachable;
change the cast to chart_spec.cast_bound::<PyDict>(py) so non-dict values fail
the cast and hit the PyValErr. Locate the block using chart_spec.cast_bound,
keep the depythonize::<ChartSpec>(&chart_spec.clone()) call for successful
dicts, and ensure the error returned remains the same
(PyValErr::new_err("chart_spec must be a string or dict")) when the cast fails.
Also keep the existing serde error handling for depythonize::<ChartSpec>
unchanged.

vegafusion-core/src/task_graph/task.rs (1)

31-40: 🧹 Nitpick | 🔵 Trivial

Consider propagating error instead of unwrap().

Line 36 uses unwrap() on try_from(). If serialization fails, this panics. Since other methods in this impl return Result, consider making new_value return Result<Self> or use expect() with context.

Proposed fix

-    pub fn new_value(variable: Variable, scope: &[u32], value: MaterializedTaskValue) -> Self {
+    pub fn new_value(variable: Variable, scope: &[u32], value: MaterializedTaskValue) -> Result<Self> {
         Self {
             variable: Some(variable),
             scope: Vec::from(scope),
             task_kind: Some(TaskKind::Value(
-                ProtoMaterializedTaskValue::try_from(&value).unwrap(),
+                ProtoMaterializedTaskValue::try_from(&value)?,
             )),
             tz_config: None,
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vegafusion-core/src/task_graph/task.rs` around lines 31 - 40, The constructor
new_value currently unwraps the result of
ProtoMaterializedTaskValue::try_from(&value) which will panic on serialization
errors; change new_value to propagate the error by returning Result<Self, E>
(matching the crate error type or using anyhow::Error) or at minimum return
Result<Self, TryFromProtoError> and replace the unwrap() with ?; update the
function signature (pub fn new_value(...) -> Result<Self, _>) and construct the
TaskKind::Value using the ?-propagated ProtoMaterializedTaskValue so callers can
handle serialization failures instead of panicking.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/build-and-test.yml:
- Around line 101-104: The CI step "Verify generated proto files are up to date"
currently runs "pixi run gen-proto-py" and then "git diff --exit-code
vegafusion-python/vegafusion/proto/" which misses untracked new files; update
this step to check both tracked diffs and untracked files by running the
existing git diff check and then failing if "git ls-files --others
--exclude-standard vegafusion-python/vegafusion/proto/" returns any output (or
equivalently test the output of "git status --porcelain
vegafusion-python/vegafusion/proto" is empty) so that newly generated untracked
proto files also cause the job to fail.

In `@automation/download_datafusion_proto.py`:
- Line 67: Replace the unnecessary f-string print call — change the print
invocation print(f"  datafusion.proto") to a normal string print(" 
datafusion.proto") (locate the print statement matching print(f" 
datafusion.proto") in automation/download_datafusion_proto.py and remove the
leading 'f').

In `@automation/gen_proto_py.py`:
- Line 32: The subprocess.run calls that assign to result (e.g., the call at the
line producing "result = subprocess.run(cmd, capture_output=True, text=True)"
and the similar call near line 95) manually check result.returncode; update both
subprocess.run invocations to include the explicit argument check=False so the
intent to handle return codes manually is clear and lint warnings are suppressed
(leave the existing manual returncode checks in place). Locate the calls in
automation/gen_proto_py.py by searching for "subprocess.run(" and add
check=False to those call sites.

In `@CLAUDE.md`:
- Line 5: The Markdown headings need blank lines above and below them; update
the file so "## Architecture" (and the other headings referenced at the same
locations) have an empty line immediately before and after the heading, and
apply the same fix for the headings at the other indicated positions (including
the ones mentioned for lines 19, 21, 27, and 33) so every heading is separated
by a blank line above and below.

In `@vegafusion-common/CLAUDE.md`:
- Line 5: Add a blank line immediately after each Markdown heading in CLAUDE.md
(e.g., the "## Key Modules" heading and the other headings noted at lines 21 and
27) so that there is a single empty line between the heading and the following
content; update each heading region to insert one newline below the heading to
satisfy MD022.

In `@vegafusion-core/CLAUDE.md`:
- Around line 30-35: Update the proto modification workflow in CLAUDE.md to
include the Python proto-generation step: after editing the .proto file and
running `cargo build -p vegafusion-core` and rebuilding dependents (e.g., `pixi
run dev-py`), also run the Python generator (`pixi run gen-proto-py` or the
exact generator command) to regenerate files under
vegafusion-python/vegafusion/proto/* so checked-in Python protobufs stay in sync
with Rust-generated code; mention build.rs as the Rust generator and instruct
contributors to rebuild both Rust and Python protobuf artifacts.

In `@vegafusion-core/src/runtime/runtime.rs`:
- Around line 406-422: The code currently pushes a PreTransformValuesWarning for
every table when row_limit is Some even if no truncation occurs; update the loop
that matches (row_limit, MaterializedTaskValue::Table(table)) to first check
whether the table actually exceeds the limit (e.g. if table.num_rows() >
row_limit or table.len() > row_limit depending on the table API) and only then
push the PreTransformValuesWarning (ValuesWarningType::RowLimit /
PreTransformRowLimitWarning) and return
MaterializedTaskValue::Table(table.head(row_limit)); otherwise leave
materialized_value unchanged and do not push a warning before pushing into
task_values.

In `@vegafusion-core/src/task_graph/task_value.rs`:
- Around line 99-116: The TryFrom implementation for MaterializedTaskValue calls
value.data.as_ref().unwrap(), which can panic on malformed
ProtoMaterializedTaskValue; update the try_from(&ProtoMaterializedTaskValue)
implementation to explicitly handle None by returning an appropriate
VegaFusionError instead of unwrapping (check for
value.data.as_ref().ok_or_else(...) or match on value.data.as_ref()), then
proceed with the existing MaterializedTaskValueData::Table and ::Scalar
branches; reference the TryFrom impl for MaterializedTaskValue, the try_from
function, and ProtoMaterializedTaskValue.data when making this change.

In `@vegafusion-python/checks/check_lazy_imports.py`:
- Around line 29-32: The print statements in check_lazy_imports.py that warn
about skipping the narwhals lazy import use implicit string concatenation so the
"{narwhals.__version__}" placeholder is not interpolated; update the offending
print calls (the one that constructs the warning message referencing
narwhals.__version__) to use an f-string by adding the leading f before the
second string literal so the {narwhals.__version__} is evaluated and the actual
version is printed.

In `@vegafusion-python/CLAUDE.md`:
- Around line 5-6: Update the CLAUDE.md "Structure" and build notes to reflect
that the crate is no longer just a single lib.rs with PyChartState: mention the
added pure-Python resolver and dataset modules and the generated
vegafusion/proto/* files, and add a proto-regeneration step (e.g., run the proto
codegen) alongside the existing "pixi run dev-py" instruction so readers know to
regenerate protos before running; reference lib.rs, PyChartState, the
resolver/dataset modules, vegafusion/proto/*, and the "pixi run dev-py" command
in the updated text.

In `@vegafusion-python/pyproject.toml`:
- Line 2: The distribution name in pyproject.toml is set to "vegafusion", which
conflicts with the upstream package; change the package distribution name (the
"name" field in pyproject.toml) back to the fork-specific value (e.g.,
"deepnote_vegafusion") so installs target deepnote_vegafusion while imports can
remain vegafusion; update the "name" entry accordingly to avoid colliding with
the upstream identity.

In `@vegafusion-python/src/plan_resolver.rs`:
- Around line 35-64: The code currently calls Python::attach directly inside the
async resolve_plan path (using Python::attach in the async task), which blocks
the Tokio worker; change resolve_plan to perform all Python GIL work inside a
blocking closure (e.g., tokio::task::spawn_blocking) instead of calling
Python::attach inline. Use the resolver's thread_safe flag (self.thread_safe or
the existing runtime selection logic) to decide whether to use spawn_blocking
for non-thread-safe resolvers or a non-blocking path for thread-safe ones, and
move/clone the py_resolver into the blocking closure and call Python::attach (or
acquire GIL) there; follow the pattern in vendor.rs spawn_blocking usage as a
reference. Ensure you only mutate or read Python objects inside that blocking
closure and return plain Rust types back to the async context.

In `@vegafusion-python/src/utils.rs`:
- Around line 27-45: The code branch that reconstructs an ExternalTableProvider
from inline_dataset is dropping the ExternalDataset.source field; extract the
source (e.g., let source: Option<String> =
inline_dataset.getattr("source")?.extract()?) from inline_dataset and preserve
it when creating the provider (either by passing it into
ExternalTableProvider::new or calling the provider's setter/constructor that
accepts source) so the reconstructed provider created in the provider variable
retains the original source value.

In `@vegafusion-python/tests/test_jupyter_widget.py`:
- Around line 464-466: The current retry on get_canvas() uses tenacity to retry
any exception for 30s; narrow it to only retry transient Selenium lookup errors
(e.g., NoSuchElementException and StaleElementReferenceException) by adding a
retry=retry_if_exception_type(...) clause to the decorator and import those
exceptions from selenium.common.exceptions, and make the stop timeout
configurable (use a fixture or an environment variable like CANVAS_RETRY_TIMEOUT
and pass int(...) into stop_after_delay) so long-running parametrized test runs
don't accumulate long delays.

In `@vegafusion-python/tests/test_plan_resolver.py`:
- Line 22: Tests introduced new ruff violations (ARG001/ARG002/RUF059/ANN401)
mainly from unused parameters and overly broad Any annotations; update the
signatures and variables to prefix intentionally unused names with an underscore
(e.g., change setup_module(module: Any) -> None to setup_module(_module: Any) ->
None) and for places where the resolver protocol requires dynamic types, replace
broad Any with a narrower type if possible or add a targeted inline noqa comment
(# noqa: ANN401) on that specific annotation; apply the same pattern to the
other flagged locations (lines referenced: 52, 94-99, 227-228, 289-290, 320-323,
371-372, 389-390, 443-446, 504-508, 576-577, 625-630, 688-689), and ensure
unused import warnings like warnings are renamed to _warnings or prefixed
similarly to satisfy ARG001/ARG002/RUF059.
- Around line 188-195: Replace the use of a fixed temp filename (the csv_path
variable created with tempfile.gettempdir() + "vf_resolver_test.csv") with a
per-test temporary file; e.g., use the pytest tmp_path fixture (or
tempfile.NamedTemporaryFile) to create a unique path and pass that to
pcsv.write_csv when creating the table in test_plan_resolver.py (update the
csv_path assignment and any test teardown accordingly). Apply the same change to
the other occurrences noted (the blocks around the csv usage at the other
occurrences referenced) so each test uses its own tmp_path/NamEDTemporaryFile
instead of a shared file.

In `@vegafusion-python/tests/test_spark_e2e.py`:
- Around line 137-139: The test sorts both inmemory_df and spark_df using
sort_cols which can yield inconsistent ordering for NaN values; update the two
calls to inmemory_df.sort_values(...) and spark_df.sort_values(...) to pass an
explicit na_position argument (e.g., na_position='last') so NaN ordering is
deterministic across implementations when comparing results.

In `@vegafusion-python/vegafusion/dataset.py`:
- Line 13: The __slots__ declaration is unsorted and triggers RUF023; update the
__slots__ tuple in the class (the "__slots__" symbol in this file) to have its
entries sorted alphabetically (e.g., "__weakref__" before "data") so the tuple
becomes sorted and ruff-clean.
- Around line 60-64: The current branch skips registering live data when
self._metadata already contains "_vf_ref_id", causing stale/missing registry
entries; change the logic in ExternalDataset initialization so that whenever
data is provided you always create a new _DataRef(data) and store it into
ExternalDataset._registry under the existing "_vf_ref_id" if present (reuse that
id) or under a newly generated uuid if not, then ensure
self._metadata["_vf_ref_id"] is set to that id; update references to _DataRef,
ExternalDataset._registry, and self._metadata["_vf_ref_id"] accordingly.

In `@vegafusion-python/vegafusion/plan_resolver.py`:
- Around line 147-197: The recursive walker _resolve_external_tables can
infinite-recurse on cyclic plans; modify its signature to accept an optional
visited set (e.g., visited: set[int] = None) and/or a depth counter with a
max_depth default, initialize visited if None, and at each call check and add
id(node) (or another stable node identity) before recursing; if id(node) is
already in visited or depth exceeds max_depth, return immediately. Update all
recursive calls (including the initial/public caller) to pass the visited
set/depth along so cycles are detected and traversal stops safely (references:
_resolve_external_tables, LogicalPlanNode, _get_child_nodes, sidecar).
- Around line 40-43: Replace the typing.Union usage for the ResolutionResult
alias with the modern pipe union syntax: change ResolutionResult = Union[Table,
ResolvedPlan] to use Table | ResolvedPlan, and ensure the file has from
__future__ import annotations at the top so forward references/type annotations
work; keep the same comment about the return semantics and update any references
in resolve_plan / resolve_plan_proto to the new alias if needed.
- Around line 200-210: The function _extract_table_name has unnecessary chained
elifs after returns; simplify control flow by replacing the elif branches with
plain if statements following the initial if so each branch returns immediately
without using elif/else, keeping the same conditions that use
table_ref.WhichOneof("table_reference_enum") and the same ValueError fallback
for unknown variants; ensure the function still handles "bare", "partial", and
"full" and raises the same ValueError when which is None or unexpected.

In `@vegafusion-runtime/CLAUDE.md`:
- Around line 5-85: The markdown file CLAUDE.md has lint warnings due to missing
blank lines before headings and around fenced code blocks; fix by inserting a
single blank line before each top-level and subsection heading (e.g., "Key
Modules", "Patterns", "Async Transforms", "Custom Functions", "Testing",
"Feature Flags", "Arrow String Type Handling") and ensure there is a blank line
both before and after each fenced code block (e.g., the Rust examples under
"When checking DataType", "When pattern matching ScalarValue", and "When
downcasting ArrayRef") so that all headings and ``` fenced blocks are separated
by a blank line from surrounding text.

In
`@vegafusion-runtime/src/expression/compiler/builtin_functions/data/vl_selection_test.rs`:
- Around line 545-604: Add a regression test that verifies FieldSpec::try_from
accepts all three Arrow string types: create a test function (in the same tests
mod) that constructs ScalarValue::Utf8, ScalarValue::LargeUtf8, and
ScalarValue::Utf8View with the string values that should map to the respective
FieldSpec variants, call FieldSpec::try_from on each, and assert the returned
value matches the expected FieldSpec variant (use matches! like the existing
tests for Op and SelectionType).

In `@vegafusion-runtime/src/expression/compiler/config.rs`:
- Around line 21-31: The Default impl for CompilationConfig currently does a
heavy DataFusion setup (make_datafusion_context + ResolverPipeline::new) which
causes wasted allocations; change Default to avoid creating a real pipeline
(e.g., set pipeline to a lightweight placeholder or make it an Option::None) and
add a new explicit constructor (e.g., CompilationConfig::with_pipeline(...) or
CompilationConfig::lightweight()) for cases that need a real ResolverPipeline;
then update build_compilation_config to stop relying on ..Default::default() and
instead set the pipeline explicitly using the new constructor or by passing a
real ResolverPipeline created with make_datafusion_context so the expensive
make_datafusion_context/ResolverPipeline::new call is only performed when
actually needed.

In `@vegafusion-runtime/src/sql/spark.rs`:
- Around line 473-478: The match over ast::BinaryOperator currently captures a
_case_insensitive flag for PGRegexIMatch and PGRegexNotIMatch but never uses it;
update the code path that builds the Spark expression (after the match that
defines negate and _case_insensitive) to pass a third flags argument to Spark's
regexp_like when _case_insensitive is true (e.g., supply "i"), and ensure the
negate behavior (from negate) remains applied; look for the match that defines
(negate, _case_insensitive) and the code that generates regexp_like to add the
conditional flags argument so case-insensitive operators (~* and !~*) behave
correctly.

In `@vegafusion-runtime/tests/test_spark_sql.rs`:
- Around line 346-372: The test currently uses regexp_like() directly so it
never triggers rewrite_pg_regex; change the test to create a filter that uses
the Postgres regex operator (e.g., the `~` or `~*` operator) instead of calling
regexp_like() directly, then run logical_plan_to_spark_sql(&plan) and assert
that the generated SQL contains regexp_like(...) and does not contain the `~`
operator; update assertions around regexp_like, the absence of " ~ " and the
expected_sql to match the rewritten form so rewrite_pg_regex is exercised and
validated.

In `@vegafusion-server/CLAUDE.md`:
- Around line 1-23: The markdown lacks required blank lines around headings and
code blocks; update the document so each heading (e.g., "Build", "CLI", "Feature
Flags", "Pitfalls") is preceded and followed by a blank line and ensure the
fenced code block for the build commands has a blank line before the opening
```bash and after the closing ``` to satisfy markdownlint rules; keep the same
content and ordering, only add the missing blank lines around the headings and
the code block.

---

Outside diff comments:
In `@vegafusion-core/build.rs`:
- Around line 31-43: Extract the list of proto paths into a single shared
constant or accessor (e.g. a const PROTOS: &[&str] or fn proto_files() ->
Vec<&'static str>) and use that symbol from both main() and gen_tonic() instead
of repeating the hard-coded array; update the
tonic_prost_build::configure().compile_protos call to pass that shared PROTOS
and ensure any other code that previously duplicated the list references the
same symbol so additions/removals only happen in one place.

In `@vegafusion-core/src/task_graph/task.rs`:
- Around line 31-40: The constructor new_value currently unwraps the result of
ProtoMaterializedTaskValue::try_from(&value) which will panic on serialization
errors; change new_value to propagate the error by returning Result<Self, E>
(matching the crate error type or using anyhow::Error) or at minimum return
Result<Self, TryFromProtoError> and replace the unwrap() with ?; update the
function signature (pub fn new_value(...) -> Result<Self, _>) and construct the
TaskKind::Value using the ?-propagated ProtoMaterializedTaskValue so callers can
handle serialization failures instead of panicking.

In `@vegafusion-python/src/utils.rs`:
- Around line 96-104: The branch that tries to handle a dict currently uses
chart_spec.cast_bound::<PyAny>(py) which always succeeds and makes the final
"must be a string or dict" fallback unreachable; change the cast to
chart_spec.cast_bound::<PyDict>(py) so non-dict values fail the cast and hit the
PyValErr. Locate the block using chart_spec.cast_bound, keep the
depythonize::<ChartSpec>(&chart_spec.clone()) call for successful dicts, and
ensure the error returned remains the same (PyValErr::new_err("chart_spec must
be a string or dict")) when the cast fails. Also keep the existing serde error
handling for depythonize::<ChartSpec> unchanged.

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: aea57135-cb5b-4702-b597-8ff475607758

📥 Commits

Reviewing files that changed from the base of the PR and between 3a5ea1d and ccbfd64.

⛔ Files ignored due to path filters (3)

• Cargo.lock is excluded by !**/*.lock
• pixi.lock is excluded by !**/*.lock
• vegafusion-wasm/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (113)

• .gitattributes
• .github/workflows/build-and-test.yml
• CLAUDE.md
• Cargo.toml
• automation/download_datafusion_proto.py
• automation/gen_proto_py.py
• examples/rust-examples/Cargo.toml
• examples/rust-examples/examples/custom_resolver.rs
• pixi.toml
• ruff.toml
• vegafusion-common/CLAUDE.md
• vegafusion-common/Cargo.toml
• vegafusion-common/src/data/json_writer.rs
• vegafusion-common/src/data/table.rs
• vegafusion-common/src/datatypes.rs
• vegafusion-common/src/error.rs
• vegafusion-core/CLAUDE.md
• vegafusion-core/Cargo.toml
• vegafusion-core/build.rs
• vegafusion-core/src/data/tasks.rs
• vegafusion-core/src/expression/ast/expression.rs
• vegafusion-core/src/planning/apply_pre_transform.rs
• vegafusion-core/src/planning/projection_pushdown.rs
• vegafusion-core/src/planning/stringify_local_datetimes.rs
• vegafusion-core/src/planning/watch.rs
• vegafusion-core/src/proto/pretransform.proto
• vegafusion-core/src/runtime/mod.rs
• vegafusion-core/src/runtime/plan_executor.rs
• vegafusion-core/src/runtime/plan_resolver.rs
• vegafusion-core/src/runtime/runtime.rs
• vegafusion-core/src/spec/chart.rs
• vegafusion-core/src/spec/data.rs
• vegafusion-core/src/spec/visitors.rs
• vegafusion-core/src/task_graph/memory.rs
• vegafusion-core/src/task_graph/task.rs
• vegafusion-core/src/task_graph/task_value.rs
• vegafusion-core/src/transform/pipeline.rs
• vegafusion-core/src/transform/sequence.rs
• vegafusion-core/src/variable/mod.rs
• vegafusion-python/.mypy.ini
• vegafusion-python/CLAUDE.md
• vegafusion-python/Cargo.toml
• vegafusion-python/checks/check_lazy_imports.py
• vegafusion-python/proto/SOURCE.txt
• vegafusion-python/proto/datafusion.proto
• vegafusion-python/proto/datafusion/proto-common/proto/datafusion_common.proto
• vegafusion-python/pyproject.toml
• vegafusion-python/src/chart_state.rs
• vegafusion-python/src/lib.rs
• vegafusion-python/src/plan_resolver.rs
• vegafusion-python/src/unparse.rs
• vegafusion-python/src/utils.rs
• vegafusion-python/src/vendor.rs
• vegafusion-python/tests/test_jupyter_widget.py
• vegafusion-python/tests/test_plan_resolver.py
• vegafusion-python/tests/test_spark_e2e.py
• vegafusion-python/vegafusion/__init__.py
• vegafusion-python/vegafusion/dataset.py
• vegafusion-python/vegafusion/plan_resolver.py
• vegafusion-python/vegafusion/proto/__init__.py
• vegafusion-python/vegafusion/proto/datafusion/__init__.py
• vegafusion-python/vegafusion/proto/datafusion/proto_common/__init__.py
• vegafusion-python/vegafusion/proto/datafusion/proto_common/proto/__init__.py
• vegafusion-python/vegafusion/proto/datafusion/proto_common/proto/datafusion_common_pb2.py
• vegafusion-python/vegafusion/proto/datafusion_pb2.py
• vegafusion-python/vegafusion/runtime.py
• vegafusion-runtime/CLAUDE.md
• vegafusion-runtime/Cargo.toml
• vegafusion-runtime/src/data/codec.rs
• vegafusion-runtime/src/data/datafusion_resolver.rs
• vegafusion-runtime/src/data/external_table.rs
• vegafusion-runtime/src/data/inline_table.rs
• vegafusion-runtime/src/data/mod.rs
• vegafusion-runtime/src/data/pipeline.rs
• vegafusion-runtime/src/data/tasks.rs
• vegafusion-runtime/src/datafusion/udfs/datetime/make_timestamptz.rs
• vegafusion-runtime/src/datafusion/udfs/datetime/timeunit.rs
• vegafusion-runtime/src/expression/compiler/binary.rs
• vegafusion-runtime/src/expression/compiler/builtin_functions/data/vl_selection_test.rs
• vegafusion-runtime/src/expression/compiler/call.rs
• vegafusion-runtime/src/expression/compiler/config.rs
• vegafusion-runtime/src/lib.rs
• vegafusion-runtime/src/plan_executor.rs
• vegafusion-runtime/src/signal/mod.rs
• vegafusion-runtime/src/sql/spark.rs
• vegafusion-runtime/src/task_graph/runtime.rs
• vegafusion-runtime/src/task_graph/task.rs
• vegafusion-runtime/src/transform/aggregate.rs
• vegafusion-runtime/src/transform/collect.rs
• vegafusion-runtime/src/transform/extent.rs
• vegafusion-runtime/src/transform/fold.rs
• vegafusion-runtime/src/transform/identifier.rs
• vegafusion-runtime/src/transform/impute.rs
• vegafusion-runtime/src/transform/pivot.rs
• vegafusion-runtime/src/transform/stack.rs
• vegafusion-runtime/src/transform/window.rs
• vegafusion-runtime/tests/test_expression_evaluation.rs
• vegafusion-runtime/tests/test_image_comparison.rs
• vegafusion-runtime/tests/test_plan_executor.rs
• vegafusion-runtime/tests/test_plan_resolver.rs
• vegafusion-runtime/tests/test_planning.rs
• vegafusion-runtime/tests/test_spark_sql.rs
• vegafusion-runtime/tests/test_task_graph_runtime.rs
• vegafusion-runtime/tests/test_transform_pivot.rs
• vegafusion-runtime/tests/util/equality.rs
• vegafusion-server/CLAUDE.md
• vegafusion-server/Cargo.toml
• vegafusion-server/build.rs
• vegafusion-server/src/main.rs
• vegafusion-server/tests/test_task_graph_runtime.rs
• vegafusion-wasm/CLAUDE.md
• vegafusion-wasm/Cargo.toml
• vegafusion-wasm/package.json

💤 Files with no reviewable changes (7)

• vegafusion-core/src/runtime/plan_executor.rs
• vegafusion-runtime/src/lib.rs
• vegafusion-runtime/tests/test_plan_executor.rs
• vegafusion-runtime/src/plan_executor.rs
• vegafusion-common/src/error.rs
• vegafusion-core/src/variable/mod.rs
• vegafusion-runtime/tests/test_planning.rs

[OlegWock]

Fork-only changes in this file

[OlegWock]

Newer version of DataFusion, in some cases returned rows were in different order, so sorting them before comparing is required. This shouldn't affect charting results

[OlegWock]

Fork-only changes in this file

[OlegWock]

Fork-only changes in this file

[OlegWock]

@coderabbitai resolve and pause reviews

[coderabbitai]

@OlegWock Sure, resolving all comments and pausing reviews.

✅ Actions performed

Comments resolved and changes approved.

---

Reviews paused.

[OlegWock]

@coderabbitai approve

[coderabbitai]

✅ Actions performed

Comments resolved and changes approved.

[m1so]

LGTM, left one minor comment about redundant asserts in regex rewrite test

[OlegWock]

@coderabbitai approve

[coderabbitai]

✅ Actions performed

Comments resolved and changes approved.