chore(operators): Add proper descriptions to text source fields - #6804
Conversation
Automated Reviewer SuggestionsBased on the
|
|
@mengw15 Could you please look over this when you get the chance? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6804 +/- ##
=========================================
Coverage 78.96% 78.96%
Complexity 3798 3798
=========================================
Files 1161 1161
Lines 46084 46084
Branches 5110 5110
=========================================
Hits 36388 36388
Misses 8076 8076
Partials 1620 1620
*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 | 498 | 0.304 | 19,961/24,230/24,230 us | 🟢 -15.1% / 🔴 +59.7% |
| 🔴 | bs=100 sw=10 sl=64 | 999 | 0.61 | 99,152/125,702/125,702 us | 🔴 +14.9% / 🔴 +15.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,178 | 0.719 | 849,218/893,096/893,096 us | ⚪ within ±5% / 🟢 +17.3% |
Baseline details
Latest main 49f9e2c from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 498 tuples/sec | 515 tuples/sec | 767.9 tuples/sec | -3.3% | -35.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.304 MB/s | 0.314 MB/s | 0.469 MB/s | -3.2% | -35.1% |
| bs=10 sw=10 sl=64 | p50 | 19,961 us | 18,437 us | 12,502 us | +8.3% | +59.7% |
| bs=10 sw=10 sl=64 | p95 | 24,230 us | 28,543 us | 16,234 us | -15.1% | +49.3% |
| bs=10 sw=10 sl=64 | p99 | 24,230 us | 28,543 us | 18,919 us | -15.1% | +28.1% |
| bs=100 sw=10 sl=64 | throughput | 999 tuples/sec | 1,062 tuples/sec | 974.8 tuples/sec | -5.9% | +2.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.61 MB/s | 0.648 MB/s | 0.595 MB/s | -5.9% | +2.5% |
| bs=100 sw=10 sl=64 | p50 | 99,152 us | 93,672 us | 102,449 us | +5.9% | -3.2% |
| bs=100 sw=10 sl=64 | p95 | 125,702 us | 109,369 us | 108,652 us | +14.9% | +15.7% |
| bs=100 sw=10 sl=64 | p99 | 125,702 us | 109,369 us | 116,310 us | +14.9% | +8.1% |
| bs=1000 sw=10 sl=64 | throughput | 1,178 tuples/sec | 1,154 tuples/sec | 1,004 tuples/sec | +2.1% | +17.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.719 MB/s | 0.705 MB/s | 0.613 MB/s | +2.0% | +17.3% |
| bs=1000 sw=10 sl=64 | p50 | 849,218 us | 869,307 us | 999,606 us | -2.3% | -15.0% |
| bs=1000 sw=10 sl=64 | p95 | 893,096 us | 902,045 us | 1,046,770 us | -1.0% | -14.7% |
| bs=1000 sw=10 sl=64 | p99 | 893,096 us | 902,045 us | 1,076,937 us | -1.0% | -17.1% |
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,401.60,200,128000,498,0.304,19961.47,24230.20,24230.20
1,100,10,64,20,2002.00,2000,1280000,999,0.610,99152.07,125701.53,125701.53
2,1000,10,64,20,16973.74,20000,12800000,1178,0.719,849217.89,893096.36,893096.36|
Good direction. One consistency note: the existing ~100 @JsonPropertyDescriptions are short declarative phrases ("Data column for box plot"), whereas these are questions / multi-sentence ("What is the name of the column…?"). Worth aligning to the short-phrase style and keeping only the ones that add real info (the per-line vs whole-file note on attributeType is useful; "Attribute Name" probably doesn't need a sentence). @aglinxinyuan for input. |
|
Please fix the format of the file. |
There was a problem hiding this comment.
Pull request overview
This PR improves operator documentation for the text source operators by adding richer field-level descriptions (via @JsonPropertyDescription) and updating the property editor styling so multi-line descriptions render cleanly in the Angular UI.
Changes:
- Added
@JsonPropertyDescriptionannotations toTextSourceOpDesc(attribute type/name, limit, offset) andTextInputSourceOpDesc(text input). - Tweaked the property editor SCSS to preserve newlines in form “extra” text (
.ant-form-item-extra) rendered vianzExtra. - Updated some field titles (e.g., “Limit (lines)”, “Offset (lines)”) to be more explicit.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.scss | Preserve newlines and improve spacing for multi-line property descriptions in the form UI. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/text/TextSourceOpDesc.scala | Add field descriptions/titles for shared text source properties (attribute type/name, limit, offset). |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/text/TextInputSourceOpDesc.scala | Add a description for the text input field shown in the operator property editor. |
Comments suppressed due to low confidence (1)
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/text/TextSourceOpDesc.scala:66
- Same as above for Offset: "large binary" is a single-tuple type but the HideAnnotation regex does not hide Offset for it, even though the executor ignores offset/limit when attributeType.isSingle.
@JsonPropertyDescription("Number of lines to skip from the start before reading. " +
"(Not used when reading the whole file as one row.)")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com>
|
The deferred To recover: re-run the failed jobs in the run above, or manually dispatch the Codecov Upload workflow with |
Yicong-Huang
left a comment
There was a problem hiding this comment.
LGTM with comments on the description itself.
|
Flaky tests are fun! |
|
@Yicong-Huang can you merge this pls? |
Let's fix flaky tests! |
|
Thanks, @Neilk1021, merging. |
|
@Yicong-Huang It looks like the github-merge-queue bot dropped the PR due to a status check timeout. Could you please re-add it to the merge queue when you get a chance? |
What changes were proposed in this PR?
Added descriptions to fields in
TextSourceOpDescandTextInputSourceOpDesc. Currently these operators suffer heavily from a lack of information.Any related issues, documentation, discussions?
Closes #6802. Related idea in #6798
How was this PR tested?
Deployed changes locally via docker and
./bin/local-dev.sh upWas this PR authored or co-authored using generative AI tooling?
No.