Skip to content

feat: add support for directory scan mode and enhance file path handling - #674

Open
Edwardvaneechoud wants to merge 6 commits into
mainfrom
feature/add-glob-read
Open

feat: add support for directory scan mode and enhance file path handling#674
Edwardvaneechoud wants to merge 6 commits into
mainfrom
feature/add-glob-read

Conversation

@Edwardvaneechoud

Copy link
Copy Markdown
Owner

This pull request introduces comprehensive support for directory-mode file reading and schema inference throughout the codebase, improving robustness and flexibility when handling multiple files as a single logical dataset. The changes include new utility functions for expanding glob patterns, validation checks for directory scans, and refactoring of code generation and data engine logic to support these features. Additionally, the code now better handles edge cases, such as zero matched files and inclusion of source-path columns.

The most important changes are:

Directory Scan Support and Validation

  • Added validation checks (assert_directory_scan_supported) in code generation, data engine, and flow graph logic to ensure that directory scans are only allowed for supported file types and encodings, preventing runtime errors. [1] [2] [3] [4] [5]
  • Worker processes now explicitly refuse to execute directory scans, raising a clear error if attempted.

Glob Pattern Expansion and Source Resolution

  • Introduced _resolve_scan_source and related helpers to expand glob patterns for directory-mode reads, ensuring that file lists are resolved before passing to polars, and raising a specific error if no files match.
  • Updated CSV, Parquet, and IPC readers to use the resolved file list or path, and to support the optional inclusion of source-path columns. [1] [2]

Code Generation Enhancements

  • Refactored code generation to emit correct polars or FlowFrame directory scan code, including new helper methods for rendering argument lists and handling encoding-specific logic. [1] [2] [3]
  • Ensured that directory-mode reads in generated code are editable and round-trippable, mirroring engine behavior.

Schema Inference for Directory Mode

  • Added a schema callback for directory-mode reads that probes the first matching file to infer the schema, handling zero matches gracefully and ensuring no duplicate columns are predicted.

Robustness and Consistency Improvements

  • Updated logic to consistently lowercase encodings when checking for UTF variants, preventing mismatches between directory and worker code paths.

These changes collectively make directory-based data ingestion safer, more predictable, and easier to maintain across the codebase.

@netlify

netlify Bot commented Aug 16, 2026

Copy link
Copy Markdown

Deploy Preview for flowfile-wasm ready!

Name Link
🔨 Latest commit f9abfc9
🔍 Latest deploy log https://app.netlify.com/projects/flowfile-wasm/deploys/6a8346c80ade70000801b52c
😎 Deploy Preview https://deploy-preview-674--flowfile-wasm.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@Edwardvaneechoud
Edwardvaneechoud marked this pull request as ready for review August 16, 2026 15:11
@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 6m 32s —— View job


Claude's review of #674

Todo list

  • Read root CLAUDE.md
  • Read package-level CLAUDE.md for touched packages (flowfile_core, flowfile_frame, flowfile_frontend, flowfile_worker, shared)
  • Fetch PR diff (git diff a459eb2..HEAD, the actual branch point — origin/main...HEAD has no common ancestor in this shallow checkout)
  • Review correctness (directory scan validation, glob expansion, schema inference, cross-package contracts)
  • Review conventions (Polars usage, Pydantic v2, Vue 3/Pinia, comment style)
  • Review security (path handling, worker refusal of directory scans)
  • Review test coverage
  • Post final review summary

Summary

This is a well-scoped, thoroughly-tested feature. The design consistently gates directory-scan support at three independent layers (flow_graph.add_read, FlowDataEngine.create_from_path, and the code generator), all routed through the single shared/path_utils.py::assert_directory_scan_supported — good defense in depth, and shared/path_utils.py stays dependency-light (glob/os/re only), per the shared package's import-only-downward contract.

Cross-package contract compliance:

  • The worker explicitly refuses directory-mode reads (create_from_path_worker raises DirectoryScanUnsupportedError), and assert_directory_scan_supported only allows the UTF-8-csv/parquet/ipc combinations that add_read's _func routes to FlowDataEngine.create_from_path in-core rather than to the worker — so directory scans structurally never reach the worker. Backed by test_directory_refusal_backstop_in_create_from_path and the drift test.
  • flowfile_core.schemas.input_schema.ReceivedTable and flowfile_worker.create.models.ReceivedTable are two hand-duplicated Pydantic models; flowfile_core/tests/test_received_table_drift.py is a nice addition that guards the two new fields (scan_mode, include_file_paths) don't silently drift apart across the core↔worker wire boundary.
  • Core still never .collect()s — directory reads build a pl.LazyFrame via pl.scan_csv/scan_parquet/scan_ipc over the resolved file list, same as the single-file path.
  • flowfile_frame's public API surface changed (scan_mode/include_file_paths on read_csv/read_parquet/read_ipc/scan_csv/scan_parquet/scan_ipc) and flow_frame_methods.pyi was regenerated/committed — stub gate honored.

Correctness highlights:

  • Change detection (SourceFileInfo) is nicely thought through: the snapshot is taken before the scan and only committed on success, file_count is None-safe for legacy saved state, and a changed/deleted source now outranks a cache hit (with tests pinning the new decision ordering).
  • The parameter-substitution interaction (${dir} directory reads) is subtle — apply_parameters_in_place substitutes both path and the already-glob-suffixed abs_file_path in place before _decide_execution runs — and it's correctly covered by test_param_pattern_run_skips_when_unchanged.
  • Zero-match handling is consistent between the run path (NoFilesMatchedError) and the schema-prediction path (empty schema, tolerated at settings-save time) — both intentional and tested.
  • The Polars-export code generator repeats the engine's own glob expansion (documented as necessary since Polars' own globbing differs in dotfile/ordering behavior), while the FlowFrame export instead re-emits scan_mode="directory" so the round-trip stays editable — both paths have dedicated parity tests (test_directory_read_codegen.py) that execute the generated code and diff it against the live node's own output.

Two non-blocking scalability notes (left inline):

  1. flowfile_core/flowfile_core/flowfile/flow_node/executor.py_source_file_changedSourceFileInfo.has_changed() does a full recursive glob + per-file os.stat() on every execution decision for a directory-mode read node, before the cache-hit check. For directories with very large file counts this runs synchronously in core on every run, even ones that would otherwise be free cache hits.
  2. flowfile_core/flowfile_core/flowfile/flow_data_engine/create/funcs.py_assert_uniform_columns opens every matched parquet/ipc file to read its schema before the real scan; an O(n) metadata pass for large file counts.

Both look like accepted "slice 1" tradeoffs (the test suite explicitly frames this as slice 1 of the feature, e.g. csv dtype-widening prediction is a documented known gap), so these are flagged as forward-looking considerations rather than blockers.

Tests: Coverage is extensive and good — 473+292+241+109+228 new lines across engine behavior, codegen round-trip, executor/state change-detection, cross-package drift, and flowfile_frame reader inference, including edge cases (zero matches, schema divergence, parameter substitution, legacy state deserialization).

Not reviewed / limitations of this pass:

  • I could not execute the test suite in this sandboxed session (poetry run pytest ... requires interactive approval I don't have here) — I read the tests carefully but did not run them; please confirm CI is green.
  • flowfile_wasm isn't touched by this PR — if directory-mode reads are meant to work in the browser/Pyodide build too, that's presumably a follow-up rather than in scope here.

No correctness, security, or convention-breaking issues found.
· Branch: feature/add-glob-read

# on disk changes, so a cache hit here would serve data the user just replaced.
if self._source_file_changed(state):
strategy = self._determine_strategy(run_location)
return ExecutionDecision(True, strategy, InvalidationReason.SOURCE_FILE_CHANGED)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalability note (non-blocking): _source_file_changed runs on every _decide_execution call for a read node, and for directory-mode sources it delegates to SourceFileInfo.has_changed()_aggregate_stats() (flow_node/state.py), which does a fresh recursive glob.glob(..., recursive=True) plus an os.stat() per matched file. This happens synchronously in core, before the cache_results/results_exists short-circuit, on every run — even a run that would otherwise be a pure cache hit.

For a directory with a very large number of files (a common case for "read every file in this folder" workflows — think 100k+ small files), this could add non-trivial latency to every single flow run and briefly block the core process, which conflicts with the "core stays lean / no heavy synchronous work" spirit in CLAUDE.md (even though this isn't a .collect()).

Given this is called out as "slice 1" in the test docstrings, this is likely an accepted tradeoff, but it might be worth a follow-up (e.g., caching the aggregate stat scan for a short TTL, or capping the walk) before this sees very large directories in the wild.

Discuss/fix →

encoding="utf8-lossy",
ignore_errors=True,
**fallback_infer,
**extra,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor note: _assert_uniform_columns opens every matched file (sequentially) just to read its schema via collect_schema() before the real scan happens. For a directory scan over many parquet/ipc files this is an O(n) metadata-read pass in addition to the actual scan. It's a reasonable correctness-first tradeoff (documented well in the docstring), but for directories with thousands of files it could meaningfully slow down every run of the node. Worth keeping in mind if a later slice adds pagination/partitioned datasets with large file counts.

@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 1m 29s —— View job


Claude's review of #674

Todo list

  • Read root CLAUDE.md
  • Check what changed since the last review (this is a synchronize trigger)
  • Fetch PR diff against origin/main
  • Review new/changed code for correctness, conventions, security, tests
  • Post final review summary

Summary

I re-checked this PR after the synchronize trigger, comparing git diff origin/main...HEAD against what was reviewed in the previous pass. The diff is byte-for-byte unchanged — every file's insertion/deletion counts match exactly what was already reviewed (e.g. code_generator.py +104/-6, funcs.py +73/-12, executor.py +60/-22, flow_graph.py +59/-2, etc., totaling the same 2424/-77 across the same 23 files). git merge-base origin/main HEAD now resolves cleanly (the branch's own merge commit 0dc7f75 pulled in origin/main, so CLAUDE.md's polars-pin update flows through as expected — no drift there). No new commits landed on top of ae594e1 since the last full pass.

Given that, my prior review of this PR stands:

  • No correctness, security, or convention-breaking issues. Directory-scan support is consistently gated at three layers (flow_graph.add_read, FlowDataEngine.create_from_path, code generator) via shared/path_utils.py::assert_directory_scan_supported, the worker structurally refuses directory-mode reads, and the ReceivedTable core/worker Pydantic duplication is guarded by a dedicated drift test.
  • Cross-package contracts hold: core still never .collect()s (directory reads build a pl.LazyFrame over the resolved file list), and the flowfile_frame public-API changes (scan_mode/include_file_paths) shipped with a regenerated .pyi stub.
  • Test coverage is thorough: 473+292+241+109+228 new lines covering engine behavior, codegen round-trip, executor/state change-detection, cross-package drift, and flowfile_frame reader inference — including zero-match, schema-divergence, and parameter-substitution edge cases.
  • Two non-blocking scalability notes were already left as inline comments on this PR (still applicable, unchanged):
    1. flowfile_core/flowfile_core/flowfile/flow_node/executor.py:210_source_file_changed runs a full recursive glob + per-file os.stat() on every execution decision for directory-mode reads, even ones that would otherwise be a cache hit.
    2. flowfile_core/flowfile_core/flowfile/flow_data_engine/create/funcs.py:209_assert_uniform_columns opens every matched parquet/ipc file to read its schema before the real scan, an O(n) metadata pass for large file counts.

Both are framed in the tests as accepted "slice 1" tradeoffs, not blockers.

Not re-verified in this pass: I still could not execute poetry run pytest ... in this sandboxed session — please confirm CI is green before merging.

No new action needed from me unless new commits are pushed or you'd like me to act on the scalability notes above.
· Branch: feature/add-glob-read

@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 3m 10s —— View job


Claude's review of #674

Todo list

  • Read root CLAUDE.md + package-level CLAUDE.md (flowfile_core, flowfile_frame, flowfile_frontend, flowfile_worker, shared)
  • Fetch PR diff (git diff origin/main...HEAD, scoped to the 23 files actually in the PR — a raw 3-dot diff against origin/main pulls in unrelated already-merged commits due to this checkout's history, so I diffed the exact path list from the PR's file list)
  • Review correctness (directory scan validation, glob expansion, schema inference, cross-package contracts)
  • Review conventions (Polars usage, Pydantic v2, Vue 3/Pinia, comment style)
  • Review security (path handling, worker refusal of directory scans)
  • Review test coverage / migrations / stubs
  • Post final review summary

Summary

I re-reviewed the current HEAD (0dc7f75, after the origin/main merge). The merge itself only touched CLAUDE.md here (a polars-pin note update) and pulled in unrelated frontend files from other merged PRs that are not part of this PR's actual diff — the substantive directory-scan code is unchanged from the prior review pass, and I independently re-verified the key contracts rather than just re-stating the earlier findings.

Design holds up well on a second pass:

  • Three independent gates (FlowGraph.add_read, FlowDataEngine.create_from_path, FlowGraphCodeConverter._handle_directory_read) all route through the single shared/path_utils.py::assert_directory_scan_supported, and flowfile_core/.../create/funcs.py::create_from_path_worker explicitly raises DirectoryScanUnsupportedError — so a directory scan structurally can never reach the worker. shared/path_utils.py stays dependency-light (glob/os/re only), per the shared package's import-only-downward contract.
  • code_generator.py's new _handle_directory_read correctly reuses the existing self.unsupported_nodes escape hatch (same pattern as the catalog-reader and other unsupported-node paths at lines 1121/1127/1176/etc.) rather than inventing a new failure path — a flow that would fail at run time is refused at export time instead of emitting broken code.
  • flow_graph.py::add_read's ordering is deliberate and correct: assert_directory_scan_supported runs on received.path before set_absolute_filepath() is called, so an unsupported file type never gets its (irrelevant) glob pattern computed first. The directory_schema_callback is installed on node.user_provided_schema_callback before node.setting_input = input_file (whose setter's reset() eagerly background-starts whatever schema callback is present) — getting this ordering backwards would have silently triggered a full throwaway directory build instead of the cheap single-file schema probe.
  • flowfile_frame's _resolve_scan_mode / ensure_glob_pattern / is_glob_pattern mirror the engine's ${param}-masking and directory/glob detection consistently between frontend inference (readFileTypes.ts::inferScanModeFromPath), the FlowFrame API, and core — including the DIRECTORY_CAPABLE_TYPES TS mirror of DIRECTORY_SCAN_FILE_TYPES (comment explicitly says so, and the sets match: csv/parquet/ipc).
  • flow_frame_methods.pyi was correctly regenerated for every new scan_mode/include_file_paths parameter on read_csv/read_parquet/read_ipc/scan_csv/scan_parquet/scan_ipcmake check_stubs should pass.
  • The executor rewrite (executor.py) correctly moves the source snapshot to before the scan (_pending_source_snapshot, committed to state.source_file_info only after a successful run) so a file written mid-run doesn't get silently folded into "no change," and it purges the worker's hash-keyed cache on SOURCE_FILE_CHANGED specifically — a source-file change doesn't rotate the node hash, so without that purge _do_execute_remote would serve a stale cached frame.

Same two non-blocking scalability notes as the previous pass on this PR still apply (unchanged code, already flagged inline):

  1. _source_file_changedSourceFileInfo.has_changed() (directory mode) does a full recursive glob + per-file os.stat() on every execution decision, before the cache-hit short-circuit — even runs that would otherwise be free cache hits. Worth a follow-up (TTL cache or a cap) before very large directories.
  2. _assert_uniform_columns in create/funcs.py opens every matched parquet/ipc file for a metadata-only schema read before the real scan — O(n) per run for large file counts.

Both remain reasonable "slice 1" tradeoffs given the test suite explicitly frames this as slice 1 (e.g. the documented gap that csv dtype-widening isn't predicted across files in the schema callback).

Not reviewed / limitations of this pass:

  • Could not execute the test suite in this sandboxed session — read the tests but didn't run them; please confirm CI is green.
  • flowfile_wasm isn't touched — presumably a follow-up if directory-mode reads should work in the browser build.

No new correctness, security, or convention issues found on this pass.
· Branch: feature/add-glob-read

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown

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


Claude's review of #674

Todo list

  • Read root CLAUDE.md
  • Check what changed since the last review pass (new commit on top of 0dc7f75)
  • Fetch and review the incremental diff
  • Review correctness / conventions / security / tests for new changes
  • Post final review summary

Summary

Since the last review pass (at 0dc7f75), one new commit landed: 57b8dea — "refine schema callback handling to optimize directory read performance." Despite the commit message, this is a comment-only change in flowfile_core/flowfile_core/flowfile/flow_graph.py (2 insertions / 5 deletions, no logic touched):

  • Two 3-line explanatory comments around the directory_schema_callback ordering in add_read (lines ~5556 and ~5599) were condensed into single-line comments.
  • This is a good change per CLAUDE.md's comment doctrine ("Keep comments minimal... No long explanatory blocks... one short line at most").
  • I diffed the surrounding code before/after: the actual logic — installing node.user_provided_schema_callback = directory_schema_callback before node.setting_input = input_file (so reset()'s eager prefetch runs the cheap single-file schema probe instead of a throwaway full directory build) — is byte-for-byte unchanged. The FlowNode(...) construction path in the else branch is likewise unchanged apart from the comment.

No new correctness, security, or convention issues — this commit is a pure documentation/comment tidy-up with zero behavioral impact, so all prior findings on this PR still stand:

  • No blockers. Directory-scan support remains consistently gated at three layers via shared/path_utils.py::assert_directory_scan_supported, the worker structurally refuses directory-mode reads, and cross-package contracts (no .collect() in core, ReceivedTable drift test, regenerated .pyi stubs) all hold.
  • Two previously-flagged non-blocking scalability notes remain unaddressed (unchanged in this commit, still worth a follow-up before very large directories):
    1. flowfile_core/flowfile_core/flowfile/flow_node/executor.py:210_source_file_changed does a full recursive glob + per-file os.stat() on every execution decision for directory-mode reads, even ones that would otherwise be a cache hit.
    2. flowfile_core/flowfile_core/flowfile/flow_data_engine/create/funcs.py:209_assert_uniform_columns opens every matched parquet/ipc file to read its schema before the real scan.

Not re-verified in this pass: couldn't execute poetry run pytest ... in this sandboxed session — please confirm CI is green before merging.
· Branch: feature/add-glob-read

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 1m 23s —— View job


Claude's review of #674

Todo list

  • Read root CLAUDE.md
  • Check what changed since the last review pass (new commit f9abfc9 on top of 57b8dea)
  • Fetch and review the incremental diff
  • Review correctness / conventions / security / tests for new changes
  • Post final review summary

Summary

Commit f9abfc9 ("Fix directory read review findings: stale downstream cache, glob escaping") directly addresses the two non-blocking scalability/correctness notes flagged in prior review passes on this PR, plus fixes a real bug. This is a solid, well-tested follow-up.

What changed and why it's correct:

  1. Stale downstream cache on source-file change (real correctness fix, not just the scalability note). flow_graph.py adds FlowGraph._refresh_read_source_freshness(), called from run_graph() right after the existing _refresh_catalog_reader_freshness(). For every read node whose SourceFileInfo.has_changed() is true, it purges the worker's node-hash-keyed cache (remove_cache()), then calls invalidate_cache() (bumps _cache_epoch, which folds into FlowNode.hash), then reset(). The ordering is load-bearing and correctly documented: remove_cache() must run while the old hash is still current, and reset() must immediately follow invalidate_cache() because any hash access in between would re-memoize the old value and the downstream cascade wouldn't fire. I traced this through flow_node.py (hash at line ~725 depends on _cache_epoch; invalidate_cache/reset at ~1766/1724) and the invariant holds.

  2. Glob metacharacter escaping. shared/path_utils.py::ensure_glob_pattern now glob.escape()s a concrete existing file (previously report[1].csv would return unescaped and get reinterpreted as a character class matching nothing), and the synthesized directory glob uses a new _case_insensitive_ext() helper that turns .csv into .[cC][sS][vV] — needed because fnmatch/glob only fold case on Windows, so without this a scan would silently skip A.CSV on Linux/macOS. New tests (test_ensure_glob_pattern_escapes_an_existing_bracketed_file, test_ensure_glob_pattern_is_idempotent, test_synthesised_extension_matches_any_casing) cover exactly these cases.

  3. glob=False on the resolved-file-list scan. funcs.py::_scan_extra_kwargs and the codegen's directory-scan emitter now pass glob=False to pl.scan_csv/scan_parquet/scan_ipc. Correct and necessary: the file list is already fully expanded by expand_glob_pattern, and without this, polars would re-interpret literal filenames containing */?/[ as patterns of their own, silently dropping matching files a second time.

  4. _assert_uniform_columns now also compares dtypes, not just column names, via a new _canonical_dtype() that normalizes only what polars itself unifies for free (struct field order, datetime time zone) — everything else (e.g. Int32 vs Int64) now fails fast at build time instead of poisoning a later collect. This is a meaningful correctness improvement over the previous name-only check, still consistent with the documented "fail fast, mirror what polars would raise" design.

  5. Worker now refuses directory-mode requests at the route layer too (flowfile_worker/routes.py::create_table, 422 before any file stat), in addition to the existing create_from_path_worker guard — belt-and-suspenders consistent with the "worker structurally never executes a directory scan" contract, with a new test (test_create_table_refuses_directory_scan_mode).

  6. is_utf8_encoding() helper consolidates the encoding check shared by assert_directory_scan_supported and flow_graph.add_read's worker-vs-core routing decision, replacing a hand-duplicated "utf" in encoding.lower() substring check — the previous version would (mildly) mismatch on hypothetical encodings just containing "utf" as a substring; now both call sites agree via one function.

  7. Digest-based change detectionSourceFileInfo gains a files_digest (sha256 over sorted (path, mtime_ns, size) triples) so a same-count/same-total-size/same-newest-mtime replacement (e.g. an rsync -a-style refresh) now reads as changed. I verified expand_glob_pattern already returns a sorted list (shared/path_utils.py:111), so the digest is deterministic across runs as documented, not incidentally order-dependent. Legacy digest-less snapshots correctly fall back to the old three-tuple comparison (from_dict/has_changed both handle files_digest is None).

  8. Docs page (reading-data.md) gained a full "Reading a Directory of Files" section — accurate against the code (case-insensitive extension matching, UTF-8-only CSV restriction, NoFilesMatchedError, dtype/name schema-mismatch behavior all correctly described).

No new issues found. All the changes in this commit are narrowly scoped to the previously-flagged concerns, come with dedicated new tests for each behavior change, and don't introduce new cross-package contract violations (worker still never executes a directory scan; core still never .collect()s the full frame; the codegen changes reuse the existing unsupported_nodes escape hatch rather than inventing a new failure path).

Not re-verified in this pass: still could not execute poetry run pytest ... in this sandboxed session — please confirm CI is green before merging.
· Branch: feature/add-glob-read

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant