Skip to content

fix(workflow-operator): fail loudly on unparsable scan rows#6323

Open
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:fix/workflow-operator-scan-silent-row-drop
Open

fix(workflow-operator): fail loudly on unparsable scan rows#6323
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:fix/workflow-operator-scan-silent-row-drop

Conversation

@eugenegujing

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

CSV/JSONL scan sources infer each column's type from only the first INFER_READ_LIMIT (= 100) rows. When a later row held a value that did not parse as the inferred type, the exec caught the error, mapped the row to null/None, and filtered it out — silently dropping the entire row with no error, warning, skipped-row count, or log. Every downstream count, aggregate, and join then ran on a silently truncated dataset. The bug is the silence, not the rejection; no comparable engine (Spark, Polars, DuckDB, Arrow) silently drops whole rows.

Before:  row fails to parse  ->  caught -> null/None  ->  filtered out   (silent data loss)
After:   row fails to parse  ->  RuntimeException      ->  console error + run stops

Changes:

  • Throw a RuntimeException (original exception kept as cause) instead of silently dropping an unparsable row, in CSVScanSourceOpExec, JSONLScanSourceOpExec, ParallelCSVScanSourceOpExec, and CSVOldScanSourceOpExec. The throw surfaces through DataProcessor.handleExecutorException as a console error and stops the run (fail-fast, matching the default of Spark FAILFAST / Polars / DuckDB / Arrow).

  • Add a shared ScanRowParseError helper that identifies the offending column by re-parsing each field and builds a message that leads with the essential facts, so it stays useful even when the console title line truncates, e.g.:

    Row 150: value '55.5' in column 'age' cannot be read as INTEGER. Column types were inferred from the first 100 rows of the file, and this value does not match. Fix the value in the file, or clean the data before scanning.

    A generic fallback covers rows with no single identifiable column (e.g. a malformed JSON line).

  • Remove the now-dead null filter in CSV and csvOld; ParallelCSV keeps its legitimate null-skip paths (exhausted block, all-null/blank line) and its filter — only parse failures abort.

Inference logic is unchanged (the 100-row sampling is untouched). Scala only — no UI change.

Behavior-change note for reviewers: workflows that previously ran to completion while silently discarding malformed rows will now fail on the first such row. This is the intended fix (loud over silent) and matches the modern-engine default, but it is a behavior change — see the discussion on #6279. Sample photo is attached below.

Any related issues, documentation, discussions?

Closes #6279

How was this PR tested?

Scan-operator unit tests (updated to assert the thrown error and message; added a JSONL malformed-line fallback test):

sbt "WorkflowOperator/testOnly \
  org.apache.texera.amber.operator.source.scan.csv.CSVScanSourceOpExecSpec \
  org.apache.texera.amber.operator.source.scan.json.JSONLScanSourceOpExecSpec \
  org.apache.texera.amber.operator.source.scan.csv.ParallelCSVScanSourceOpExecSpec \
  org.apache.texera.amber.operator.source.scan.csvOld.CSVOldScanSourceOpExecSpec"
# => Tests: succeeded 28, failed 0

Full backend gate in one clean session (lint + format + whole suite):

sbt clean "scalafixAll --check" scalafmtCheckAll test
# scalafixAll --check: pass
# scalafmtCheckAll:    pass
# test => Total number of tests run: 1721; succeeded 1721, failed 0, aborted 0 (pending 2)

Manual, in the Texera UI: scanned a CSV whose column is integer for the first 100 rows with a non-integer value (55.5) at data row 150. The CSV File Scan operator turned red and the console showed the message above; the run stopped at row 150 instead of silently emitting a short result.
Screenshot 2026-07-10 at 3 48 20 PM

Was this PR authored or co-authored using generative AI tooling?

Co-authored by: Claude Code (Fable 5)

- Throw a RuntimeException (original exception kept as cause) instead of
  silently dropping a row that does not parse into the inferred type, in
  CSVScanSourceOpExec, JSONLScanSourceOpExec, ParallelCSVScanSourceOpExec,
  and CSVOldScanSourceOpExec.
- Add ScanRowParseError helper that identifies the offending column and
  builds an actionable message (row number, value, column, expected type),
  with a generic fallback when no single column is identifiable.
- Remove the now-dead null filter in CSV and csvOld; keep ParallelCSV's
  legitimate null-skip paths and filter (only parse failures abort).
- Update the scan exec specs to assert the thrown error; add a JSONL
  malformed-line fallback test.

Closes apache#6279
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @aglinxinyuan, @Ma77Ball, @Yicong-Huang
    You can notify them by mentioning @aglinxinyuan, @Ma77Ball, @Yicong-Huang in a comment.

@codecov-commenter

codecov-commenter commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.93%. Comparing base (a6c85b3) to head (c43f9a9).
⚠️ Report is 93 commits behind head on main.

Files with missing lines Patch % Lines
...amber/operator/source/scan/ScanRowParseError.scala 89.47% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6323      +/-   ##
============================================
+ Coverage     60.64%   60.93%   +0.28%     
- Complexity     3368     3397      +29     
============================================
  Files          1133     1135       +2     
  Lines         44141    44460     +319     
  Branches       4825     4878      +53     
============================================
+ Hits          26771    27090     +319     
+ Misses        15908    15877      -31     
- Partials       1462     1493      +31     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 44.59% <ø> (ø) Carriedforward from a6c85b3
amber 66.51% <92.85%> (+0.37%) ⬆️
computing-unit-managing-service 9.97% <ø> (+9.97%) ⬆️
config-service 52.30% <ø> (ø)
file-service 65.28% <ø> (+1.31%) ⬆️
frontend 52.40% <ø> (ø) Carriedforward from a6c85b3
notebook-migration-service 78.94% <ø> (+0.37%) ⬆️
pyamber 91.18% <ø> (ø) Carriedforward from a6c85b3
workflow-compiling-service 55.14% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 4 better · 🔴 3 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 3e08ebb benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 368 0.225 25,337/38,042/38,042 us 🔴 +9.4% / 🔴 +171.0%
🔴 bs=100 sw=10 sl=64 803 0.49 126,441/133,398/133,398 us 🟢 -14.8% / 🔴 +38.3%
🟢 bs=1000 sw=10 sl=64 923 0.563 1,083,116/1,129,802/1,129,802 us 🟢 -7.9% / 🔴 +20.1%
Baseline details

Latest main 3e08ebb from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 368 tuples/sec 373 tuples/sec 845.85 tuples/sec -1.3% -56.5%
bs=10 sw=10 sl=64 MB/s 0.225 MB/s 0.228 MB/s 0.516 MB/s -1.3% -56.4%
bs=10 sw=10 sl=64 p50 25,337 us 26,598 us 11,663 us -4.7% +117.2%
bs=10 sw=10 sl=64 p95 38,042 us 34,778 us 14,037 us +9.4% +171.0%
bs=10 sw=10 sl=64 p99 38,042 us 34,778 us 18,666 us +9.4% +103.8%
bs=100 sw=10 sl=64 throughput 803 tuples/sec 811 tuples/sec 1,102 tuples/sec -1.0% -27.1%
bs=100 sw=10 sl=64 MB/s 0.49 MB/s 0.495 MB/s 0.673 MB/s -1.0% -27.2%
bs=100 sw=10 sl=64 p50 126,441 us 118,734 us 91,441 us +6.5% +38.3%
bs=100 sw=10 sl=64 p95 133,398 us 156,522 us 97,817 us -14.8% +36.4%
bs=100 sw=10 sl=64 p99 133,398 us 156,522 us 108,384 us -14.8% +23.1%
bs=1000 sw=10 sl=64 throughput 923 tuples/sec 919 tuples/sec 1,128 tuples/sec +0.4% -18.2%
bs=1000 sw=10 sl=64 MB/s 0.563 MB/s 0.561 MB/s 0.688 MB/s +0.4% -18.2%
bs=1000 sw=10 sl=64 p50 1,083,116 us 1,078,434 us 901,939 us +0.4% +20.1%
bs=1000 sw=10 sl=64 p95 1,129,802 us 1,226,498 us 940,934 us -7.9% +20.1%
bs=1000 sw=10 sl=64 p99 1,129,802 us 1,226,498 us 977,688 us -7.9% +15.6%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,543.00,200,128000,368,0.225,25337.23,38041.79,38041.79
1,100,10,64,20,2489.49,2000,1280000,803,0.490,126440.71,133398.41,133398.41
2,1000,10,64,20,21671.95,20000,12800000,923,0.563,1083116.29,1129801.82,1129801.82

@aglinxinyuan

Copy link
Copy Markdown
Contributor

Since it is a behavior change, can you open a discussion post for this?

@eugenegujing

eugenegujing commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Since it is a behavior change, can you open a discussion post for this?

Yes I have raised one.

@xuang7 xuang7 added the release/v1.2 back porting to release/v1.2 label Jul 12, 2026
@chenlica

Copy link
Copy Markdown
Contributor

Since it is a behavior change, can you open a discussion post for this?

Yes I have raised one.

@eugenegujing Please include the URL of the discussion.

@eugenegujing

eugenegujing commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

This is the discussion: #6324

@xuang7 xuang7 removed the release/v1.2 back porting to release/v1.2 label Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CSV/JSONL scan sources silently drop rows that fail type parsing

5 participants