feat(workflow-operator): export the operators that work on text - #8340
feat(workflow-operator): export the operators that work on text#8340kz930 wants to merge 26 commits into
Conversation
…ython script A workflow can be read in the editor but not taken away: there is no form of it that runs anywhere else, so a user who wants to keep a pipeline, hand it to someone without Texera, or step through it in a notebook has nothing to take. This adds the seam for one and the first few operators through it. An operator says how it reads outside the engine by implementing `StandaloneCodeGenerator`, returning a block of pandas that names its inputs and outputs as `in1df` / `out1df`. The translator walks the plan in topological order, gives every port a variable, substitutes those placeholders, and prints the leaves; `inAlldf` stands for the whole list of upstreams, which is what a variadic port like Union's needs, since any fixed count the code stated would be wrong for some workflow. An operator with no generator yet leaves a commented TODO rather than a line that looks like it works. `GET /workflow-to-python` on the compiling service returns the script for a plan it is given. Five operators implement it here — Distinct, Limit, Projection, Filter and Union — chosen to cover the shapes the translator has to handle: a single input, a config-driven one, one that renames columns, one that builds a predicate, and the variadic port. The rest of the operator set follows in later changes. `pyStringLiteral` renders a value as a Python literal with the escaping that keeps a quote or a newline in a column name from ending the literal early. The generators cannot use the runtime's decode expression, which needs an operator instance to decode through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re the files The standalone export claims that a generated script does what the operator does. Nothing checks it. This adds the two runners that make the claim checkable, and the file format they meet in. `OpExecHarness` runs a LogicalOp the way the engine does — compiling it to a physical plan and driving the executor — but outside a workflow, against JSONL files rather than a live upstream. `PyOpExecHarness` does the same for a Python operator, through the worker the engine uses. `StandaloneRunner` takes the other path: it asks the operator for its standalone code, wraps it in a script that binds `in1df` from the same files, and runs it. `TupleIO` is what the two meet in. A JSONL row carries values and no types, so the schema travels beside it in a sidecar; without one, a column written as INTEGER reads back as a number and the two paths disagree over a difference neither operator made. Both runners produce files, not assertions, so what to make of a difference is left to a later change. What is here is enough to run one operator both ways and see that the answers match, which is what the spec does with Distinct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he operator it came from The standalone export claims a generated script does what the operator does. This is what checks it, for every operator, on every configuration the operator offers. An operator is run twice. `OpExecHarness` drives it the way the engine does, compiled to a physical plan but outside a workflow, reading JSONL files rather than a live upstream; `PyOpExecHarness` does the same for a Python operator through the worker the engine uses. `StandaloneRunner` takes the other path, wrapping the operator's standalone code in a script that binds the same files. Both write files, and `Comparator` reads them back: order-insensitive by default, since the engine interleaves across workers and only the sort family promises an order. A visualization is compared as a figure rather than as a frame. What to run an operator ON is decided rather than written by hand for each. `ConfigGenerator` reads the operator's own schema — its enums, defaults, declared ranges and column pickers — and produces a base configuration plus one variant per branch the operator offers, so a switch nobody thought to try is still tried. `CanonicalFixture` is the table they run against, one column per shape an operator might ask for. `CuratedHandlers` is the escape hatch for an operator whose input cannot be derived, and `TransformVerificationRunner` decides which of the three tiers each operator takes and reports what it could not run and why. `LogicalOp.orderSensitive` and `@SampleColumn` are the two things the operators had to say for this to read them: whether row order is part of the contract, and which column a field should be pointed at when the first unused one would be a poor choice. Most of the operator set does not implement the generator yet — it arrives a family at a time — and the runner reports each of those rather than passing over it. The tier assertions for a family land with the change that gives that family its generator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-eight more operators say how they read outside the engine: the set operations, the joins, the filters and searches, the reshaping operators, the sorts, the samplers, and the control-flow pair. With these, a workflow built from anything but a visualization, a source or a model can be exported and run. Each is written against what its executor does rather than against what its name suggests, and the verification added alongside the export runs both and compares. Some of what that turned up is visible here: Distinct keeps the first occurrence because the executor's LinkedHashSet does; Filter guards every comparison with notna because a null answers false to all of them but IS NULL; the samplers seed their generator so a rerun draws the same rows. `StandaloneHelpers` holds what several of them share — the samplers' generator, the aggregate's rendering — emitted once per script rather than inlined per operator. Two executors change: Substring Search and Unnest String answered a null cell with an exception, which the script has no way to reproduce and no reason to. They now answer it the way the rest of the family does, and their specs say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (46.47%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8340 +/- ##
============================================
- Coverage 93.62% 93.46% -0.16%
- Complexity 4857 4866 +9
============================================
Files 1212 1213 +1
Lines 50037 50168 +131
Branches 6132 6136 +4
============================================
+ Hits 46847 46892 +45
- Misses 1676 1759 +83
- Partials 1514 1517 +3
*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 | 529 | 0.323 | 18,025/27,652/27,652 us | 🔴 +20.7% / 🔴 +71.4% |
| 🔴 | bs=100 sw=10 sl=64 | 1,183 | 0.722 | 82,992/104,944/104,944 us | 🔴 +7.6% / 🟢 +20.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,373 | 0.838 | 723,748/834,504/834,504 us | ⚪ within ±5% / 🟢 +35.8% |
Baseline details
Latest main 75c85aa from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 529 tuples/sec | 585 tuples/sec | 755.28 tuples/sec | -9.6% | -30.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.323 MB/s | 0.357 MB/s | 0.461 MB/s | -9.5% | -29.9% |
| bs=10 sw=10 sl=64 | p50 | 18,025 us | 14,938 us | 12,957 us | +20.7% | +39.1% |
| bs=10 sw=10 sl=64 | p95 | 27,652 us | 29,163 us | 16,134 us | -5.2% | +71.4% |
| bs=10 sw=10 sl=64 | p99 | 27,652 us | 29,163 us | 20,333 us | -5.2% | +36.0% |
| bs=100 sw=10 sl=64 | throughput | 1,183 tuples/sec | 1,230 tuples/sec | 980.1 tuples/sec | -3.8% | +20.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.722 MB/s | 0.751 MB/s | 0.598 MB/s | -3.9% | +20.7% |
| bs=100 sw=10 sl=64 | p50 | 82,992 us | 80,857 us | 101,894 us | +2.6% | -18.6% |
| bs=100 sw=10 sl=64 | p95 | 104,944 us | 97,539 us | 108,718 us | +7.6% | -3.5% |
| bs=100 sw=10 sl=64 | p99 | 104,944 us | 97,539 us | 122,482 us | +7.6% | -14.3% |
| bs=1000 sw=10 sl=64 | throughput | 1,373 tuples/sec | 1,375 tuples/sec | 1,011 tuples/sec | -0.1% | +35.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.838 MB/s | 0.839 MB/s | 0.617 MB/s | -0.1% | +35.8% |
| bs=1000 sw=10 sl=64 | p50 | 723,748 us | 719,709 us | 996,422 us | +0.6% | -27.4% |
| bs=1000 sw=10 sl=64 | p95 | 834,504 us | 844,067 us | 1,037,670 us | -1.1% | -19.6% |
| bs=1000 sw=10 sl=64 | p99 | 834,504 us | 844,067 us | 1,072,152 us | -1.1% | -22.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,377.84,200,128000,529,0.323,18024.79,27651.50,27651.50
1,100,10,64,20,1690.06,2000,1280000,1183,0.722,82992.16,104944.43,104944.43
2,1000,10,64,20,14565.61,20000,12800000,1373,0.838,723748.39,834504.37,834504.37…f the one without The split this change relies on was declared but never wired. The specs carry `@IntegrationTest` and `build.sbt` reads `WCS_TEST_FILTER` to act on it, but nothing set that variable, so the filter was a no-op and the specs that fork Python ran in the job that provisions none — failing on `No module named 'pandas'` rather than on anything they were testing. The platform job now sets `skip-integration`, which excludes them. The platform-integration job sets `integration-only` and provisions what they need: Python 3.12, amber's requirements, protoc, and the generated proto bindings, which are gitignored and so have to be regenerated before a forked driver can import pyamber. Every step is guarded on the service, so the other entries in that matrix are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…into feat/standalone-base-transforms
… have one The example was an operator another batch gives a generator to, so the assertion held only until that batch landed. A Python UDF holds whatever order these land in: its body is written by whoever drops the operator, so there is nothing for a generator to emit. The word cloud assertion goes for the same reason. It says the operator is withheld, which a later batch stops being true once its placement is seeded, and the prediction op alone already covers what the test is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything here that is not this batch's own operators belongs to apache#8327 and was carried only so the branch could compile and run its own tests before that one landed. Reviewing it twice costs more than the red build does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rows these operators add to the runner and its config tests sit in files apache#8327 introduces, so they land once that does rather than as whole new files here. Each operator's own spec stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`astype(str)` turns an empty cell into the text "nan", which a pattern or a term can match, and by the time `na=False` is consulted there is no null left for it to see. The engine answers false on a cell with nothing in it, so the script drops those rows before matching instead. Regex is where it showed: its variant over the nulls column kept one row the engine did not. Substring Search and Keyword Search had it too, and their fixtures simply never held a pattern that matches the word "nan". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`astype(str)` gets three things wrong against `toString`: an empty cell renders as the text "nan", a column holding one has become a float by then so 6 reads "6.0", and a boolean capitalises where the executor writes it lower. The comparison could not see any of it until apache#8359 stopped inferring a type per file, which is why a cast to STRING has been passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
Boolean conversion does not match Texera. Python bool of the string false is true, but Texera parses it as false. Numeric strings also differ. Please match AttributeTypeUtils and add cases for false, true, zero, one, invalid text, and null.
…ides The three sort operators here override `orderSensitive`, so the declaration they override belongs with them rather than in the change that introduces the export. Nothing in that change states an order or reads the flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The boolean conversion issue from my previous review is still present.
Python's own conversions answer differently on the values a column holds.
`bool("false")` is true, because every non-empty string is, so a script cast
"false" and "0" to true where the run it was exported from read both as false.
The numeric casts had the same shape from the other side: coercing turned
"6.7" into the integer 6 and "abc" into NaN, where the engine refuses both and
stops.
`StandaloneHelpers.AttributeCasts` transcribes `parseField`'s three numeric
and boolean arms, refusal included: a value the engine will not read now ends
the script rather than writing a number the workflow never produced.
A timestamp stays approximate and says so. The engine reads it through
DateParserUtils, whose accepted formats no single pandas call states.
The new test runs the generated Python over "true", "false", "0", "1", text
that is neither, and an empty cell, and compares each answer against
`AttributeTypeUtils.parseField` rather than against a hard-coded list, so the
two sides cannot drift apart quietly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed. The casts go through a transcription of |
carloea2
left a comment
There was a problem hiding this comment.
The boolean fix looks good. Please update this branch with current main. The build has 89 compile errors because pyStringLiteral and other old names are still used.
|
Updated with main; it was eighteen commits behind. That does not move the build though: CI checks out The 89 are |
carloea2
left a comment
There was a problem hiding this comment.
The main merge did not fix the build. pyStringLiteral is still missing and CI still has 89 compile errors.
|
This one is a draft because it cannot build alone. |
carloea2
left a comment
There was a problem hiding this comment.
I checked the dependency. These build errors come from PR 8327, which is green. The changes here look good.
carloea2
left a comment
There was a problem hiding this comment.
Reviewed alongside the related export and verification PRs. These findings are based on code inspection and focused Python checks, not a full Scala suite run.
pandas suffixes the right join key when the left frame already carries a column of that name, so dropping the bare name took the left payload and kept the key the join was supposed to remove. The frame is only known at run time, so the script picks the name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
Follow-up review of the series. Runtime findings were checked locally against the current code; deployment routing was checked from configuration.
The two keys were copied into `_iv_l` and `_iv_r` columns before the cross join, so an input column already carrying one of those names was overwritten and then dropped along with them. The merged frame already holds both keys: the left name is never renamed, and the right one takes the merge's suffix when it collides, which the script decides at run time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hape Every float that landed on a whole number was rendered as an integer, so a DOUBLE column holding 6.0 came out "6" where `toString` gives "6.0". A value cannot say which type it came from, so the helper reads the column instead: an integer dtype renders without a point, a boolean in lower case, and anything else as it stands. One case stays out of reach. A hole widens an integer column to float64 before the cast ever sees it, and its text then keeps the point the engine's integer never had. The type is lost at the reader, and no cast can recover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
Further review with local regression tests and connected execution checks.
carloea2
left a comment
There was a problem hiding this comment.
Additional boundary cases reproduced with compiled local tests.
Five operators render a column as text before matching, splitting or
storing it, and each read the value's own type to decide how. A hole
costs the column that type on the way through a file: pandas reads a
holed integer column as a float and a holed boolean one as 1.0 and 0.0,
so 6 was matched as "6.0" and true as "1.0". The declared type is the
only thing that still says which was meant, a real DOUBLE holding 6.0
looking exactly the same, so the five now take it and narrow first.
Four more answers were wrong for their own reasons, each observed
against the JVM rather than assumed:
- Keyword Search ignored its own Case Sensitive flag, always matching
without case. The base fixture is lower-case throughout, so sweeping
the flag there decided nothing.
- Type Casting emitted one line per unit, where `tupleCasting` takes a
Map and reads each column's original value once: two units naming
one column collapse to the last.
- A cast reaches `parseField(force = true)`, whose numeric branch is
NumberFormat, which truncates a decimal and drops a grouping comma
where Python's int refuses both. INTEGER and LONG cannot share one
narrowing either: Long.toInt keeps the low 32 bits while Double.toInt
saturates. A LONG read as a timestamp is milliseconds, and renders in
the JVM's zone rather than UTC.
- CONCAT skipped a leading null but not a leading empty string. The
accumulator earns its separator only once it holds something, so
"", "a", "" is "a," and not ",a,".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@carloea2 the findings here are fixed: the join key drop, the interval-join temporary names, the CONCAT fold, and the three casting cases. Would you take another look when you have a moment? |
The joins, the set operations, the sorts, the samplers and the rest go to a change of their own. What is left reads or rewrites a text column, and shares the transcription of AttributeTypeUtils that makes a cast answer as the engine does. StandaloneHelpers splits along the same line: the java.util.Random transcription follows the samplers that draw from it, and only the casts stay here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventeen operators implement the trait: the three joins, the three set operations, Aggregate, the sort family, Split, the two samplers, and If, Dummy and Sleep. What they have in common is that they rearrange rows rather than read what is inside one. A sampler decides per row whether to keep it, so which rows survive is fixed by the exact sequence java.util.Random produces. Seeding Python's own generator selects a different set, and the script would then report a different sample than the workflow it came from, so SamplingHelpers transcribes the generator rather than approximating it. Split out of apache#8340 on review, which had grown past what one reading can hold. That change keeps the operators that work on text, and the cast transcription they share. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three fields here name the fixture column that should fill them, which the type alone cannot say: a three-letter country code, a genuine price column. The annotation is test-only, read by the configuration generator and by nothing in production, but it is applied here, so it is declared here rather than where it is read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing here overrides it. The three that do are the sort family, in the relational change, so the declaration goes with them rather than being added by both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What changes were proposed in this PR?
Six operators that read or rewrite a text column implement
StandaloneCodeGenerator: Type Casting, Keyword Search, Substring Search, Unnest String, Regex and Dictionary Matcher.The sixteen that rearrange rows rather than read inside one are in #8506.
Each is written against what its executor does rather than against what its name suggests, and the verification that came with the export runs both and compares them. Some of what that turned up is visible in the code: Distinct keeps the first occurrence of a duplicate because the executor's
LinkedHashSetdoes; Filter guards every comparison withnotna, because a null answers false to all of them but IS NULL and pandas does not agree on!=; the samplers seed their generator so a rerun draws the same rows as the run it is being compared with.StandaloneHelpersholds what these six share: a transcription ofAttributeTypeUtils, emitted once near the top of a script rather than inlined per operator. A cast goes throughparseField(force = true), whose numeric branch isjava.text.NumberFormat, so the engine reads "12abc" as 12 and "false" as false where Python's own conversions answer differently.Four of these operators change what they do, not only how they export. Regex, Substring Search and Unnest String answered a null cell by raising, which a generated script has no way to reproduce and no reason to: nothing in a column matches nothing, and unnests to no rows. They answer it that way now, and their specs say so. Keyword Search states which keywords its query parser will take, which a user had no way to know from the field alone.
Any related issues, documentation, discussions?
Part of #8325, 10 of 27; that issue lists the set in order. It needs #8327 for the trait, so it does not compile until that lands, and the rows these operators add to the verification runner follow with the harness rather than as whole new files here.
Closes #8074, closes #7548. Part of #7936.
Closes #8414, the task this change is the whole of.
How was this PR tested?
Each operator asserts the block it emits in its own spec. Once the verification lands it is also run through the engine and through its generated script, on every configuration its schema offers, and the two answers compared; this branch is cut from main and does not carry that machinery, so those runs are not on this diff's CI.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)