fix(csv-scan): declare the Delimiter as the single character it is - #7261
fix(csv-scan): declare the Delimiter as the single character it is#7261kz930 wants to merge 5 commits into
Conversation
Every reader narrows customDelimiter with charAt(0), because univocity's setDelimiter and scala-csv's DefaultCSVFormat both take a Char, so characters past the first were dropped without a word: ";abc" inferred the same columns as ";", and ",;" collapsed to one exactly as a bare "," does. Closes apache#7211 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
👋 Thanks for opening this pull request, @kz930! It looks like the pull request description doesn't quite follow our template yet:
Filling out the template helps reviewers understand and triage your contribution faster. Please edit the description to complete it. This message will disappear automatically once the template is followed. You can find the template prompts by editing the description, or see CONTRIBUTING.md for the full contribution flow. |
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7261 +/- ##
=============================================
- Coverage 82.64% 72.42% -10.23%
+ Complexity 4080 2634 -1446
=============================================
Files 1162 1145 -17
Lines 46282 46215 -67
Branches 5160 5152 -8
=============================================
- Hits 38250 33470 -4780
- Misses 6341 11262 +4921
+ Partials 1691 1483 -208
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 412 | 0.252 | 23,828/27,590/27,590 us | 🔴 -11.4% / 🔴 +86.6% |
| 🔴 | bs=100 sw=10 sl=64 | 908 | 0.554 | 106,300/161,825/161,825 us | 🔴 +5.8% / 🔴 +48.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,091 | 0.666 | 914,479/960,231/960,231 us | ⚪ within ±5% / 🟢 -10.2% |
Baseline details
Latest main 436b37e from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 412 tuples/sec | 465 tuples/sec | 767.32 tuples/sec | -11.4% | -46.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.252 MB/s | 0.284 MB/s | 0.468 MB/s | -11.3% | -46.2% |
| bs=10 sw=10 sl=64 | p50 | 23,828 us | 21,862 us | 12,772 us | +9.0% | +86.6% |
| bs=10 sw=10 sl=64 | p95 | 27,590 us | 30,347 us | 15,538 us | -9.1% | +77.6% |
| bs=10 sw=10 sl=64 | p99 | 27,590 us | 30,347 us | 18,948 us | -9.1% | +45.6% |
| bs=100 sw=10 sl=64 | throughput | 908 tuples/sec | 951 tuples/sec | 972.51 tuples/sec | -4.5% | -6.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.554 MB/s | 0.581 MB/s | 0.594 MB/s | -4.6% | -6.7% |
| bs=100 sw=10 sl=64 | p50 | 106,300 us | 100,498 us | 103,020 us | +5.8% | +3.2% |
| bs=100 sw=10 sl=64 | p95 | 161,825 us | 153,328 us | 109,070 us | +5.5% | +48.4% |
| bs=100 sw=10 sl=64 | p99 | 161,825 us | 153,328 us | 118,964 us | +5.5% | +36.0% |
| bs=1000 sw=10 sl=64 | throughput | 1,091 tuples/sec | 1,097 tuples/sec | 1,005 tuples/sec | -0.5% | +8.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.666 MB/s | 0.669 MB/s | 0.613 MB/s | -0.4% | +8.6% |
| bs=1000 sw=10 sl=64 | p50 | 914,479 us | 916,384 us | 1,002,400 us | -0.2% | -8.8% |
| bs=1000 sw=10 sl=64 | p95 | 960,231 us | 974,431 us | 1,039,228 us | -1.5% | -7.6% |
| bs=1000 sw=10 sl=64 | p99 | 960,231 us | 974,431 us | 1,069,081 us | -1.5% | -10.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,485.21,200,128000,412,0.252,23827.55,27589.64,27589.64
1,100,10,64,20,2201.72,2000,1280000,908,0.554,106300.17,161824.74,161824.74
2,1000,10,64,20,18331.44,20000,12800000,1091,0.666,914479.11,960230.91,960230.91The Required Checks summary read a cancelled precheck from a superseded run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
carloea2
left a comment
There was a problem hiding this comment.
The constraint looks right. Please add a schema test for empty, one character, and multiple character delimiters so the intended compatibility behavior stays covered.
…osts The maxLength went in without a test, so nothing held the boundary or the compatibility claim. Three cases now: the schema declares a one-character string on all three scans; empty and one character are accepted while longer values are not, read off the schema rather than restating the bound; and a stored `;abc` infers exactly the columns a bare `;` does, which is why flagging those saved workflows changes how they look and not how they run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Added — |
…n check The accept/refuse test computed its own verdict from the same maxLength it had just asserted, so it restated the declaration instead of checking it -- and the validator that actually decides is ajv in the property editor, which this side cannot reach. State the bounds directly: maxLength 1, and no minLength so an empty delimiter stays valid.
Asserting the declared bound proves the schema says maxLength 1, not what a validator does with it. Feed the empty, one-character, and longer delimiters to json-schema-validator -- already a test dependency here -- against each scan's generated property schema, so the accepted and refused values are a validator's verdict rather than this test's own arithmetic.
4 participants
What changes were proposed in this pull request?
customDelimiteron the three CSV scans is declared as a string with no length constraint, while every reader narrows it withcharAt(0)— univocity'ssetDelimiterand scala-csv'sDefaultCSVFormat.delimiterboth take aChar. Each now declaresmaxLength: 1, and the description says a single character rather than leaving it implied.Why are the changes needed?
Characters past the first are dropped without a word. Driving each operator's own
sourceSchema()over a three-column;-separated file,;abcinfers the same three columns as;, and,;collapses to one column exactly as a bare,does — so the second character never participates. A workflow typed that way runs to completion and looks right, and nothing ever says part of the field was discarded.Does this PR introduce any user-facing change?
Yes, and one worth calling out. A saved workflow whose Delimiter holds more than one character will now show as invalid in the property editor, since the form validates the stored properties against the schema. It still runs — the backend does not enforce the schema — and the run behaves exactly as before, because only the first character ever counted. Surfacing those workflows is the point: today they are silently truncating.
How was this patch tested?
WorkflowOperator/compile,scalafmtCheckAll,scalafixAll --check, and the three operators' descriptor specs (22 tests, three of them added here for the constraint itself).Closes #7211
Generated-by: Claude Code (Claude Opus 5)