fix(frontend): improve static error msgs to be user-friendly for unco… - #6288
Conversation
…nfigured operators
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6288 +/- ##
============================================
+ Coverage 60.77% 60.80% +0.02%
+ Complexity 3367 3366 -1
============================================
Files 1135 1135
Lines 44191 44263 +72
Branches 4830 4848 +18
============================================
+ Hits 26855 26912 +57
- Misses 15878 15885 +7
- Partials 1458 1466 +8
*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:
|
|
@aglinxinyuan Can you please review it |
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 400 | 0.244 | 24,351/35,054/35,054 us | 🔴 +8.2% / 🔴 +131.9% |
| 🔴 | bs=100 sw=10 sl=64 | 787 | 0.48 | 125,191/158,096/158,096 us | 🔴 +23.6% / 🔴 +47.7% |
| 🟢 | bs=1000 sw=10 sl=64 | 921 | 0.562 | 1,084,863/1,128,394/1,128,394 us | 🟢 -5.3% / 🔴 +9.9% |
Baseline details
Latest main 12f9f1a from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 400 tuples/sec | 425 tuples/sec | 779.42 tuples/sec | -5.9% | -48.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.244 MB/s | 0.26 MB/s | 0.476 MB/s | -6.2% | -48.7% |
| bs=10 sw=10 sl=64 | p50 | 24,351 us | 22,513 us | 12,550 us | +8.2% | +94.0% |
| bs=10 sw=10 sl=64 | p95 | 35,054 us | 38,200 us | 15,115 us | -8.2% | +131.9% |
| bs=10 sw=10 sl=64 | p99 | 35,054 us | 38,200 us | 18,652 us | -8.2% | +87.9% |
| bs=100 sw=10 sl=64 | throughput | 787 tuples/sec | 832 tuples/sec | 1,000 tuples/sec | -5.4% | -21.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.48 MB/s | 0.508 MB/s | 0.61 MB/s | -5.5% | -21.4% |
| bs=100 sw=10 sl=64 | p50 | 125,191 us | 124,193 us | 99,872 us | +0.8% | +25.4% |
| bs=100 sw=10 sl=64 | p95 | 158,096 us | 127,890 us | 107,073 us | +23.6% | +47.7% |
| bs=100 sw=10 sl=64 | p99 | 158,096 us | 127,890 us | 114,377 us | +23.6% | +38.2% |
| bs=1000 sw=10 sl=64 | throughput | 921 tuples/sec | 911 tuples/sec | 1,020 tuples/sec | +1.1% | -9.7% |
| bs=1000 sw=10 sl=64 | MB/s | 0.562 MB/s | 0.556 MB/s | 0.622 MB/s | +1.1% | -9.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,084,863 us | 1,096,140 us | 987,499 us | -1.0% | +9.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,128,394 us | 1,191,011 us | 1,028,784 us | -5.3% | +9.7% |
| bs=1000 sw=10 sl=64 | p99 | 1,128,394 us | 1,191,011 us | 1,057,136 us | -5.3% | +6.7% |
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,500.18,200,128000,400,0.244,24351.31,35053.55,35053.55
1,100,10,64,20,2541.28,2000,1280000,787,0.480,125190.76,158095.55,158095.55
2,1000,10,64,20,21710.91,20000,12800000,921,0.562,1084863.37,1128393.66,1128393.66
You can use the bot comment to request a review. Please also fix the format issue. You can click each CI to view the issue. |
There was a problem hiding this comment.
Pull request overview
This PR improves the workflow-editing experience by replacing cryptic backend exception strings with more actionable, user-facing validation messages for common “operator added but not configured” scenarios, primarily during compilation-time schema propagation.
Changes:
- Frontend: normalize compilation-error display by stripping Java/Scala exception prefixes and grouping them under a friendlier category.
- Backend operators: add explicit validation (
require) messages for Projection and SQL/AsterixDB source configuration fields. - File-based sources: add early checks and clearer messaging when schema inference tries to read an invalid selection (e.g., folder vs. file) and when Arrow schema inference fails.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/workspace/component/result-panel/error-frame/error-frame.component.ts | Strips common exception prefixes from compilation errors and groups them for display. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/sql/SQLSourceOpDesc.scala | Adds required-field validation messages for SQL source configuration. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/sql/asterixdb/AsterixDBSourceOpDesc.scala | Adds required-field validation messages specific to AsterixDB configuration. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/json/JSONLScanSourceOpDesc.scala | Adds user-friendly validation/message when schema inference reads JSONL inputs. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csvOld/CSVOldScanSourceOpDesc.scala | Adds user-friendly validation/message when schema inference reads legacy CSV inputs. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/ParallelCSVScanSourceOpDesc.scala | Adds user-friendly validation/message when schema inference reads parallel CSV inputs. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/CSVScanSourceOpDesc.scala | Adds user-friendly validation/message when schema inference reads CSV inputs. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/arrow/ArrowSourceOpDesc.scala | Adds user-friendly validation for Arrow file selection and a clearer schema inference failure message. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/projection/ProjectionOpDesc.scala | Improves the validation message when no projection attributes are selected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/arrow/ArrowSourceOpDesc.scala:92
RootAllocatoris allocated but never closed. Since this code can run during schema inference (potentially multiple times while editing workflows), leaving the allocator open can leak off-heap memory. Consider managing it viaUsing.Managerso it’s always closed even on exceptions.
val allocator = new RootAllocator()
Using
.Manager { use =>
val channel = use(Files.newByteChannel(file.toPath, StandardOpenOption.READ))
|
@aglinxinyuan This PR is ready for final review and merge! I've gone through several rounds of Copilot AI reviews and addressed all of the architectural feedback. (Honestly, I had to stop because it kept nitpicking unrelated code until I literally hit my quota limit for reviews ). To summarize the final state of this PR: Frontend: Normalized compilation and execution errors by stripping out the raw Java/Scala exception prefixes so users get clean, readable validation messages. The local dev stack is passing all tests. Could you please take a look and approve? Thanks! |
1c7c1fd to
1af7927
Compare
1af7927 to
faa965a
Compare
aglinxinyuan
left a comment
There was a problem hiding this comment.
LGTM! I kinda agree that Copilot is nitpicking on this one.
- reformat the over-100-column require() in AsterixDBSourceOpDesc so scalafmtCheckAll passes - align the scan-source missing-fileName error in both LogicalPlan copies (amber + workflow-compiling-service) with the new 'No file selected...' wording that the updated specs expect - WorkflowCompiler: keep the first error recorded per operator so the root-cause resolution failure (e.g. FileNotFoundException with the bad path) is not overwritten by follow-on schema-propagation failures on the same operator - update workflow-operator DescSpecs that pinned the old contracts: sourceSchema() returning null before configuration (CSVOld/JSONL/MySQL/PostgreSQL/AsterixDB/SQL) and ArrowSourceOpDesc.inferSchema throwing a bare IOException
…rkflow-compiler (apache#6143) ### What changes were proposed in this PR? **This is a source-level de-duplication only — the runtime behavior of two compilations stays.** A workflow is still compiled twice: once at editing time (through workflow-compiling-service) and once at execution time (through amber). This PR only makes both paths share one compiler implementation instead of two hand-synced copies. amber and workflow-compiling-service each maintained a separate copy of the workflow compiler and its models (`WorkflowCompiler`, `LogicalPlan`, `LogicalLink`, `LogicalPlanPojo`), which had to be kept in sync by hand (e.g. apache#6288 had to patch both copies). This PR consolidates them into a single shared `common/workflow-compiler` module that both services depend on; the old per-service copies are removed. The unified `WorkflowCompiler.compile` returns a `WorkflowCompilationResult` (logical plan, optional physical plan, per-port output schemas, per-operator errors, and the output ports needing storage) and takes a `CompilationErrorHandling` mode: `Lenient` for the editing-time path (accumulate per-operator errors so the UI can render them) and `Strict` for the execution path (fail-fast before a run). amber's engine `Workflow` becomes a thin wrapper over the compilation result, built via `Workflow.fromCompilationResult`. Two known minor behavior changes (both flagged in review, both intentional): - Plan expansion now uses `scala.util.Try` (non-fatal only) where the old compiling-service caught `Throwable`, so a fatal JVM error during expansion now fails the `/compile` request instead of being recorded per-operator. - `Strict` now also fails fast on schema-propagation errors (e.g. a Projection on a missing column). The legacy execution path silently launched such plans and they only failed at runtime; per review discussion the fail-fast contract now covers this case too, pinned by a test. ### Any related issues, documentation, discussions? Closes apache#6142. Design rationale lives in the scaladoc of `WorkflowCompiler`. ### How was this PR tested? - Migrated the compiler and model unit specs into the new module and added strict-mode coverage (success path and the schema-propagation fail-fast path). - amber and workflow-compiling-service compile and test against the shared library. - Rebased over apache#6288 and verified its error-message changes carry over into the shared module (they now exist in one place instead of two). ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Opus 4.8 (Claude Code); review follow-ups by Claude Fable 5 (Claude Code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What changes were proposed in this PR?
This PR overhauls the static error validation and error messaging across Texera operators to drastically improve the experience for new users during workflow editing.
Previously, if a user dragged an operator (like
Projection,MySQL, orCSV File Scan) onto the canvas without configuring it yet, the system would immediately throw intimidating and cryptic Java exceptions (e.g.,IllegalArgumentException,SQLException: No suitable driver found, or/texera (Is a directory)).Changes proposed:
error-frame.component.tsto cleanly strip internal Java prefixes (likejava.lang.IllegalArgumentException:orrequirement failed:) so the user only sees the human-readable part of the error message.ProjectionOpDescto explicitly prompt the user to select an attribute.SQLSourceOpDesc(andAsterixDBSourceOpDesc) to intercept empty database configuration fields and return actionable UI instructions (e.g., "Please enter a valid host name for the database in the properties panel").ParallelCSV,JSONL,Arrow, etc.) to gracefully catch directory selections and missing files before attempting physical reads, replacing IOExceptions with clear prompts to select a valid file from the right-hand panel.Any related issues, documentation, discussions?
Closes #2735
How was this PR tested?
Tested manually by rebuilding the
workflow-compiling-serviceand dragging every modified operator (Projection, MySQL, PostgreSQL, AsterixDB, CSV, JSONL, Arrow) onto a new workflow canvas. Verified that the Result Panel correctly displays the newly formatted, user-friendly instructions instead of raw Java exceptions.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Antigravity (DeepMind)
Tests:
