Skip to content

feat(csv-scan): keep a numeric column's type when one of its cells is blank - #7568

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
kz930:fix/7550-csv-blank-cell-type
Aug 14, 2026
Merged

feat(csv-scan): keep a numeric column's type when one of its cells is blank#7568
aglinxinyuan merged 2 commits into
apache:mainfrom
kz930:fix/7550-csv-blank-cell-type

Conversation

@kz930

@kz930 kz930 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Schema inference and execution disagreed about what a blank cell is, and the schema side was the one that lost information. Inference set nullValue("") on its parser, so a blank read as an empty string: tryParseDouble("") fails, tryParseBoolean("") fails, and inferField lands on tryParseString(). One empty cell was enough to type a whole numeric column as STRING. Execution builds its parser without nullValue, so the same blank read as null there, which is what AttributeTypeUtils.parseField is written to pass through.

The effect reaches well past the scan. Every downstream operator that does arithmetic on such a column then receives strings and fails on rows whose values are perfectly good numbers, not on the blank one. Hugging Face Iris Logistic Regression on a three-row file fails at the first row, where numpy is handed array([['2.6', '0.75']], dtype='<U32').

Dropping the setting leaves both sides reading a blank as null, and tryParseDouble(null) already answers DOUBLE, so the column keeps the type its values give it.

One corner changes with it: a column that is blank in every sampled row now infers as INTEGER rather than STRING. Its values are null either way, so this renames the empty rather than reinterpreting anything.

Any related issues, documentation, discussions?

Closes #7550

How was this PR tested?

CSVScanSourceOpDescSpec gains a case that writes a two-column file whose numeric column is blank on one row and asserts the inferred type is DOUBLE. It fails on the previous behavior, 16 passed / 1 failed before the change and 17 / 0 after. Since this touches inference every scan goes through, the whole module was run as well: 2187 passed, 0 failed.

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

Generated-by: Claude Code (Claude Opus 5)

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 11, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 11, 2026 20:45
@github-actions

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

… blank

Schema inference and execution disagreed about what a blank cell is, and the
schema side was the one that lost information. Inference set nullValue("") on its
parser, so a blank read as an empty string: tryParseDouble("") fails,
tryParseBoolean("") fails, and inferField lands on tryParseString(). One empty
cell was enough to type a whole numeric column as STRING. Execution builds its
parser without nullValue, so the same blank read as null there, which is what
AttributeTypeUtils.parseField is written to pass through.

The effect reaches well past the scan. Every downstream operator that does
arithmetic on such a column then receives strings and fails on rows whose values
are perfectly good numbers, not on the blank one. Hugging Face Iris Logistic
Regression on a three-row file fails at the first row, where numpy is handed
array([['2.6', '0.75']], dtype='<U32').

Dropping the setting leaves both sides reading a blank as null, and
tryParseDouble(null) already answers DOUBLE, so the column keeps the type its
values give it. One corner changes with it: a column that is blank in every
sampled row now infers as INTEGER rather than STRING. Its values are null either
way, so this renames the empty rather than reinterpreting anything.

CSVScanSourceOpDescSpec gains the case; it fails on the previous behavior. The
module's 2187 tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kz930
kz930 force-pushed the fix/7550-csv-blank-cell-type branch from 2b08597 to 9f49d90 Compare August 11, 2026 20:51
@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:

  • No candidates found from git blame history.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 324278e 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 531 0.324 18,012/27,103/27,103 us 🟢 -6.8% / 🔴 +65.8%
🔴 bs=100 sw=10 sl=64 1,098 0.67 89,673/102,715/102,715 us 🟢 -20.8% / 🟢 -11.8%
🔴 bs=1000 sw=10 sl=64 1,270 0.775 779,973/875,982/875,982 us 🔴 +5.6% / 🟢 +23.3%
Baseline details

Latest main 324278e from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 531 tuples/sec 557 tuples/sec 774.89 tuples/sec -4.7% -31.5%
bs=10 sw=10 sl=64 MB/s 0.324 MB/s 0.34 MB/s 0.473 MB/s -4.7% -31.5%
bs=10 sw=10 sl=64 p50 18,012 us 18,017 us 12,738 us -0.0% +41.4%
bs=10 sw=10 sl=64 p95 27,103 us 29,069 us 16,348 us -6.8% +65.8%
bs=10 sw=10 sl=64 p99 27,103 us 29,069 us 18,848 us -6.8% +43.8%
bs=100 sw=10 sl=64 throughput 1,098 tuples/sec 1,114 tuples/sec 1,005 tuples/sec -1.4% +9.3%
bs=100 sw=10 sl=64 MB/s 0.67 MB/s 0.68 MB/s 0.613 MB/s -1.5% +9.3%
bs=100 sw=10 sl=64 p50 89,673 us 85,057 us 100,970 us +5.4% -11.2%
bs=100 sw=10 sl=64 p95 102,715 us 129,610 us 107,605 us -20.8% -4.5%
bs=100 sw=10 sl=64 p99 102,715 us 129,610 us 116,429 us -20.8% -11.8%
bs=1000 sw=10 sl=64 throughput 1,270 tuples/sec 1,300 tuples/sec 1,030 tuples/sec -2.3% +23.3%
bs=1000 sw=10 sl=64 MB/s 0.775 MB/s 0.794 MB/s 0.629 MB/s -2.4% +23.2%
bs=1000 sw=10 sl=64 p50 779,973 us 768,157 us 991,433 us +1.5% -21.3%
bs=1000 sw=10 sl=64 p95 875,982 us 829,181 us 1,036,668 us +5.6% -15.5%
bs=1000 sw=10 sl=64 p99 875,982 us 829,181 us 1,070,470 us +5.6% -18.2%
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,376.40,200,128000,531,0.324,18012.06,27102.55,27102.55
1,100,10,64,20,1821.38,2000,1280000,1098,0.670,89672.63,102714.92,102714.92
2,1000,10,64,20,15742.36,20000,12800000,1270,0.775,779972.68,875982.32,875982.32

@kz930 kz930 changed the title fix(csv-scan): keep a numeric column's type when one of its cells is blank feat(csv-scan): keep a numeric column's type when one of its cells is blank Aug 11, 2026
@codecov-commenter

codecov-commenter commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (324278e) to head (24f2b96).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7568      +/-   ##
============================================
- Coverage     90.14%   90.13%   -0.02%     
+ Complexity     4417     4413       -4     
============================================
  Files          1174     1174              
  Lines         46964    46963       -1     
  Branches       5262     5262              
============================================
- Hits          42334    42328       -6     
- Misses         2876     2877       +1     
- Partials       1754     1758       +4     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 98.62% <ø> (ø) Carriedforward from d420e20
amber 86.71% <ø> (-0.03%) ⬇️
computing-unit-managing-service 72.46% <ø> (ø)
config-service 77.31% <ø> (ø)
file-service 68.90% <ø> (ø)
frontend 91.44% <ø> (ø) Carriedforward from d420e20
notebook-migration-service 78.89% <ø> (ø)
pyamber 97.57% <ø> (ø) Carriedforward from d420e20
workflow-compiling-service 57.89% <ø> (ø)

*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.

@xuang7 xuang7 removed fix release/v1.2 back porting to release/v1.2 labels Aug 11, 2026
@xuang7
xuang7 removed their request for review August 11, 2026 21:04
@kz930

kz930 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@aglinxinyuan May you take a look at this?

@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Aug 13, 2026
@aglinxinyuan
aglinxinyuan removed this pull request from the merge queue due to a manual request Aug 13, 2026
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Aug 13, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 13, 2026
@aglinxinyuan
aglinxinyuan enabled auto-merge August 14, 2026 08:02
@github-actions github-actions Bot added the fix label Aug 14, 2026
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Aug 14, 2026
Merged via the queue into apache:main with commit c37f6ab Aug 14, 2026
31 checks passed
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.

A blank cell makes CSV File Scan infer a numeric column as STRING, breaking every downstream numeric operator

5 participants