[Edwardvaneechoud]

No description provided.

[netlify]

✅ Deploy Preview for flowfile-wasm canceled.

Name	Link
🔨 Latest commit	148ce95
🔍 Latest deploy log	https://app.netlify.com/projects/flowfile-wasm/deploys/6ae8f200a1e8aef99ec8f19b

[claude]

Claude finished @Edwardvaneechoud's task in 43s —— View job

---

Review of PR #874 (Polars 2 / Python 3.14)

This is a read-only review. I did not run the test suite or the linters. I also couldn't get a merge base against origin/main in this checkout, so I compared the two trees with a two-dot diff and read only part of the 100 changed files. The parts I read were the migration, the virtual-table and catalog changes, and the codegen changes.

Summary: The direction is sound. A virtual table now reads through its producer flow, and a cached plan serialized by one Polars version is no longer loaded by another. I found no blocking bugs in what I read. The points below are questions or small risks.

Read and looks fine

• Migration 039 (alembic/versions/039_drop_virtual_table_plan_cache.py): it is guarded with _has_column, uses batch_alter_table (SQLite-safe), has a working downgrade, and keeps the numeric prefix sequence after 038. models.py drops serialized_lazy_frame to match.
• catalog_resolution.py: serialized_lf is replaced by a _producer_fingerprint (sha256 of the producer flow file). A missing or unreadable file gives None, which means "unfingerprintable". That matches the "complete-or-nothing" doctrine in CLAUDE.md, so the node is re-run rather than served stale.
• Core contract: the removed code is all in core and it adds no .collect(). The deleted collect_threaded and get_join_count were unused core-side collects.
• Codegen:
  • polars_join_how maps outer to full only for the pl dialect, and FlowFrame keeps Flowfile's name.
  • _membership_numbers types is_in members to the column dtype. This one is subtle, because dropping a non-integer member from an integer column's is_in list matches the == chain.
  • with_row_count becomes with_row_index.

Points to check

1. Behaviour change for lazy virtual tables. Previously an optimized virtual table with current sources read its cached plan and never ran the producer. Now every read runs the producer's writer node (virtual_tables.py::held_virtual_flow_table), and with the compute protocol on that means a worker round-trip per read. This is intended, but it is a performance regression for dashboards or SQL views that read many lazy virtual tables. Confirm that is acceptable, and that docs/users/visual-editor/catalog/virtual-tables.md states it (the docs are touched in this PR).
2. polars_plan is now written for non-lazy virtual tables too. _handle_virtual_table_write calls _readable_plan(graph, writer_node), which calls render_flow, before the if optimize: branch. Previously only lazy tables did any plan work. If render_flow raises on an unusual graph (for example a custom node, or an edge whose source is outside the branch), the virtual-table write now fails where it didn't before. I'd wrap it so the write still succeeds with polars_plan=None, or add a test with a gate or custom-node producer.
3. Fingerprint only sees the saved file. _producer_fingerprint hashes registration.flow_path. An unsaved in-memory edit of the producer is invisible to it. This matches how reads resolve the producer, so it is probably fine, but it deserves a one-line note next to the function.
4. Silent None on OSError. If the producer file can't be read, the fingerprint is None with no log. A debug log would help when someone asks why a reader re-runs on every execution.
5. kernel_image_manifest.json and images.py/manager.py changed. CLAUDE.md says any kernel image input change needs make bump-version-kernel (tools/check_kernel_release_needed.py gates it in CI). I couldn't confirm that here. Check that check_kernel_manifest and check_kernel_release_needed are green.
6. Stubs: expr.pyi and flow_frame.pyi are updated. Confirm make check_stubs passes against Polars 2.1 on CI, because the stubs are generated and the diff is large (+111/-98).
7. _console_source.py: the __annotate__ exclusion is the right 3.14 fix. Make sure a 3.14 CI leg exercises it, since 3.10–3.13 never hit that path.
8. SQL views: result.collect_schema() was added after sql_context.execute so errors surface early. That is fine, but it resolves the schema eagerly, and with the protocol on, CLAUDE.md says no plan is resolved in core. collect_schema on a LazyFrame is a planner-only operation, but for scans it can touch object storage. Confirm this path only runs where core already resolved schemas (it did before via execute).

Tests and housekeeping

• Test updates are broad. I'd still add a regression test for point 2 and one for the fingerprint rotating when the producer file changes.
• Docs and the skill files are updated to match the version bump (3.10 dropped, 3.14 added). Skim README.md, CONTRIBUTING.md and docs/installation.md for any leftover "3.10" claim.

[claude]

Claude finished @Edwardvaneechoud's task in 16s —— View job

---

Review of PR #874 (Polars 2 / Python 3.14), updated for the latest push

This is a read-only review. I did not run tests or linters. I read the virtual-table, catalog and codegen changes, and I did not read all 100 files.

Summary: I found no blocking issues. The change reads a virtual table through its producer flow instead of a cached serialized plan, so a plan written by one Polars version is never loaded by another. Core still has no new .collect().

Two earlier concerns are already handled in the branch

• Plan drawing failures: _readable_plan (flow_graph/catalog_write.py:567) catches any exception from render_flow, logs a warning and returns None. A producer with unusual nodes can't fail the write.
• Fingerprint scope: _producer_fingerprint (flow_graph/catalog_resolution.py:47) now says in its docstring that it hashes the saved file only.

Still open

1. Performance of lazy virtual tables: every read now runs the producer's writer node. With the compute protocol on, that is a worker round-trip per read, where before a cached plan was read directly. This looks intended. Please confirm it is acceptable for dashboards and SQL views that read many lazy virtual tables. virtual-tables.md should say it plainly.
2. Silent None fingerprint: when the producer file can't be read (OSError), the fingerprint is None with no log. A logger.debug there would explain why a reader re-runs on every execution.
3. Kernel gate: kernel/images.py, manager.py and kernel_image_manifest.json changed. Check that make check_kernel_manifest and tools/check_kernel_release_needed.py are green. A kernel version bump (make bump-version-kernel) may be needed.
4. Stubs: expr.pyi and flow_frame.pyi changed by about 200 lines. Confirm make check_stubs passes on CI against Polars 2.1.
5. Python 3.14 coverage: the _console_source.py __annotate__ exclusion only runs on 3.14. Make sure a 3.14 CI leg exercises it.
6. collect_schema() in SQL views: I flagged this earlier and did not re-check it. It is a planner-only call, but for scans it can touch object storage. Confirm it only runs where core already resolved schemas.

Tests

• Add a regression test that the fingerprint changes when the producer file changes.
• Add a test that a virtual-table write still succeeds when render_flow raises. That would cover the None fallback above.
• Grep README.md, CONTRIBUTING.md and docs/installation.md for any remaining "3.10" claim.

[claude]

Claude finished @Edwardvaneechoud's task in 18s —— View job

---

Review of PR #874 (Polars 2 / Python 3.14), latest push

This is a read-only review. I did not run tests, linters or CI gates. I spot-checked the open items from the earlier reviews against the current branch. I did not re-read all 100 files.

Summary: I found no blocking issues.

Resolved since the earlier reviews

• Plan drawing failures: _readable_plan catches render_flow errors and returns None, so a producer with unusual nodes can't fail the write.
• Fingerprint scope: the docstring of _producer_fingerprint (flow_graph/catalog_resolution.py:47) now says it hashes the saved file only.
• Silent None fingerprint: an unreadable producer file now logs at debug level (catalog_resolution.py, except OSError).
• Leftover "3.10" claims: a grep of README.md, CONTRIBUTING.md and docs/installation.md finds none.
• collect_schema() in SQL views: the only core-side call is virtual_tables.py:383, on the SQL result right after execute. It is planner-only and runs on a path that already resolved schemas, so I consider it fine.

Still worth confirming

1. Cost of reading lazy virtual tables: every read now runs the producer's writer node, which is a worker round-trip with the compute protocol on. This looks intended. Please confirm it's acceptable for dashboards and SQL views that read many lazy virtual tables. virtual-tables.md was updated, but check that it says this plainly.
2. Kernel gate: kernel/images.py, kernel/manager.py and kernel_image_manifest.json changed. Confirm make check_kernel_manifest and tools/check_kernel_release_needed.py are green. A make bump-version-kernel may be needed.
3. Stubs: expr.pyi and flow_frame.pyi changed by about 200 lines. Confirm make check_stubs passes on CI against Polars 2.1.
4. Python 3.14 coverage: the _console_source.py __annotate__ exclusion only runs on 3.14. Make sure a 3.14 CI leg exercises it.

Tests

• I'd add a regression test that the fingerprint changes when the producer file changes.
• I'd also add one that a virtual-table write still succeeds, with polars_plan=None, when render_flow raises.