Repository navigation
Conversation
The runner discovers the operators that implement the trait, configures each from its schema, runs it through the engine and through its generated script, and compares. An operator that cannot be run is reported as a row with the reason rather than passed over, since an operator missing from the report is a fact the report has to carry. `OperatorBehaviorSpec` is what turns that into tests: one per operator per configuration its schema offers, named so a single operator can be run alone. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8362 +/- ##
============================================
- Coverage 91.75% 91.74% -0.01%
Complexity 5024 5024
============================================
Files 1281 1281
Lines 56754 56727 -27
Branches 7454 7449 -5
============================================
- Hits 52075 52047 -28
Misses 2802 2802
- Partials 1877 1878 +1
*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:
|
Three examples where one carries the point, and a cross-reference the reader can follow without being told to. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…erator's issue The row pointed at the issue for Substring Search and Unnest String, which this operator was never part of. That issue closes with the change that answers those two, and this one still raises. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The row said the platform raised on an empty cell. The operator answers one now, so the variant runs like every other. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…re in Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Both had been withheld for drawing a different picture on every run, which stopped being true once their placement was seeded. The registration is inert until the operator implements the trait, so it can sit here rather than making the change that gives them their generator depend on this one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Which tier an operator lands in is the one thing the per-operator runs do not check, so it is what this spec is for. Every visualization, the scorer, an estimator and an advanced trainer each state where they route and why that is not the tier they used to be in. All of them land before this change does, so the assertions hold when it merges rather than needing to be added back afterwards. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
One assertion per line, like every other test here. The loop it replaced was the only one of its kind in the file. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
One said the coverage report prints the withheld runs together. It no longer prints them, so what is left is the reason that does not depend on a reader: one table means the next exemption has an obvious home. The other pointed at `TransformHandler.unfillableVariants`, which does not exist. What it meant is `nullsKeepFilled`, the one thing a handler still says about its own table rather than about its operator. `withheldRunsFor` was public for the report that read it. Its only caller now is beside it, so it says so. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
carloea2
left a comment
There was a problem hiding this comment.
The transform verification runner looks good.
The configuration generator produces it; this is the label a withholding would have to name. A paragraph the one below it already carried goes with it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
e450a54 to
f1e3d14
Compare
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.
a8a3653 to
eb8b5aa
Compare
Every other verify spec runs one operator's fragment with the harness binding its placeholders, so a fault that only appears once fragments share a script and a directory has nowhere to show. Three of those are pinned here: a column named after a placeholder, two charts writing one file, and a branch that ends the process the other branches still need. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
eb8b5aa to
b052093
Compare
|
@carloea2 Since your approval the canonical table carries edge values (doubles Java writes in E notation, years outside pandas' range, digit strings), and Bullet and Gauge are compared by every figure they write. |
Co-Authored-By: Claude Opus 5.5 <[email protected]>
carloea2
left a comment
There was a problem hiding this comment.
Read the runner, routing rules and whole-plan tests. With the exact prerequisite implementations supplied locally, 41 tests pass: 29 canonical-fixture checks, all 8 translated plans, and 4 executions through the actual runner. Those exercise native and generated Limit and Union variants, plus real Python UDF and exported Pie and Contour variants, including nulls, empty input and error pages. Formatting passes.
sbt -batch 'WorkflowCompilingService/testOnly *CanonicalFixtureSpec *TranslatedPlanRunSpec *RunnerExecutionReviewSpec -- -oDF' 'scalafmtCheckAll'
No new runner-specific finding. The branch alone cannot compile yet, and the other routing assertions still need their operator export prerequisites. This is not merge ready until those and CI are cleared.
…ts DOUBLE columns A specialDoubles run beside nulls and emptyTable. Withheld, with the reason, for the 56 sklearn operators and the three charts whose library refuses a NaN or an infinity on both paths; the two dummy classifiers never read a feature and run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Every withheld run is ByDesign now; nothing reads PendingFix. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The six plans that the engine can also run move from TranslatedPlanRunSpec to amber's WorkflowParitySpec, which runs each in the engine and compares every leaf with the script's. File Scan, which needs an upstream to name its files, joins them. The two plans only the script can answer stay where they were. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@carloea2 the end-to-end layer now also runs each plan in the Texera engine and compares every leaf with the script's output ( |
A withheld run whose reason is that both paths raise now runs, and passes only when each does: a script that answers where the engine refuses is as wrong as a different answer. Every such row held, but one: the dummy classifier fits on nothing, into a model that cannot predict, so its empty table stays withheld with that reason. Co-Authored-By: Claude Opus 5.5 <[email protected]>
They run, so the list of runs not run was the wrong home for them. Each list now says what it holds: variantsNotRun the runs that cannot be set up, variantsBothRefuse the runs both paths must refuse. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Read over the shared table's JSON, an archive or a typed line has no answer, so both paths must refuse those. A large binary needs object storage, which the harness does not give the executor, so it is not run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The plans read the shared table as the CSV the CSV source reads, whole and with a hole in every column, in place of a few hand-written rows each. Union now joins two halves that share no row, so a list naming one upstream twice still fails. The chain projects before the sort, since the engine pauses a Python operator on a LONG column that holds a null and a value past 2^53. Co-Authored-By: Claude Opus 5.5 <[email protected]>
`views` holds counts past 2^31 on both sides of an int's limit, and two that fit one, so a knob that admits LONG reads a LONG column. Bubble Chart's size refuses its negative counts on both sides. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Bubble Chart's x axis, which has no type rule, now takes the LONG column first, so the bubble size keeps its own column and both sides draw the chart. The run is compared like any other. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@carloea2 Six commits since I last asked: the runs both paths must refuse now have their own list, the workflow plans run on the shared table, and the table gained a LONG column |
carloea2
left a comment
There was a problem hiding this comment.
The new WorkflowParitySpec does run the actual engine and translated script for connected and branched plans, which addresses my earlier architecture concern. I am holding approval of this head until the fixture cycle noted on #8363 is split and the #8358 and #8359 harness findings are fixed. I have not run the new whole-engine suite on this combined stack, so the earlier local results do not verify this head.
build.sbt now makes amber's Test / compile depend on WorkflowCompilingService / Test / compile. The local-dev parser read that as a project named WorkflowCompilingService/Test/compile, and the slash in it ended every bin/local-dev.sh command with an unbound variable. An argument holding a slash names a task, so it is skipped. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…apache#8366) ### What changes were proposed in this PR? Nine of them, and what they have in common is that the output is not a plot drawn from numbers: a table rendered as a figure, a range a reader moves, an image or an HTML document passed through, and the waterfall, which reads as a chart but is built by accumulating rows rather than plotting them. The waterfall plots every row and appends the total as a bar of its own, rather than consuming the last row to make one. One of these also changes what the engine does. URL Visualizer wrote the cell straight into the iframe's `src`, so a quote in it closed the attribute and left the rest of the cell standing as attributes of the iframe. Both paths escape it now, the same five characters in the same order. Image Visualizer's exported script reads a string as ready base64 only where the column was declared BINARY, which is where the executor's own `b64encode` would have accepted it. ### Any related issues, documentation, discussions? Part of apache#8325, 15 of 27; that issue lists the set in order. It needs the Plotly mixin from apache#8365, so it does not compile until that lands. Closes apache#7974. Closes apache#8418, the task this change is the whole of. ### How was this PR tested? Each operator asserts the block it emits in its own spec. All nine have also been run through the engine and through their generated scripts, on every configuration their schemas offer, on a branch that carries this change together with the verification runner from apache#8362, and the two answers agreed for each of the nine. The rows these operators add to that runner arrive with it rather than as whole new files here. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
The workflow plans write their CSV with CanonicalSourceFixture, which lived in the source runner apache#8364 adds, while that runner calls the TransformVerificationRunner added here. Neither change compiled alone. The fixture now sits in CanonicalFixture.scala, unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
CanonicalSourceFixture now lives in apache#8362, which this change already needs, so the two no longer each need the other. It is removed here with the imports only it used. Co-Authored-By: Claude Opus 5.5 <[email protected]>
📊 Arrow Flight E2E bench(no arrow-flight-e2e.csv in artifact) |
…ry-operator Co-Authored-By: Claude Opus 5.5 <[email protected]>
The compiler now takes the resolver its caller supplies for a scan source's file. Both plan specs read local CSVs, so they pass the fixture resolver main's own tests use. Co-Authored-By: Claude Opus 5.5 <[email protected]>
What changes were proposed in this PR?
The runner discovers the operators that implement the trait, configures each
from its schema, runs it through the engine and through its generated script,
and compares. An operator that cannot be run is reported as a row with the
reason rather than passed over, since an operator missing from the report is
a fact the report has to carry.
OperatorBehaviorSpecis what turns that into tests: one per operator perconfiguration its schema offers, named so a single operator can be run alone.
Every operator reads one canonical table, and it now also carries the values a type reaches only at its edges. That means a double at 1e7 and past it, and one under 1e-3, which Java writes in E notation. It means timestamps in 1600, 2262, 2500 and 9999, outside pandas' nanosecond range. And it means text of digits with leading zeros, which a reader must not take for a number. Without them, a difference that only shows at those values is never asked about. It also carries
views, its one LONG column, with counts past 2^31 on both sides of an int's limit.Bullet and Gauge are compared by the figures their script writes, now that it writes every one rather than the first.
A whole workflow is compared too.
WorkflowParitySpec, in amber's integration tests, runs each plan in the engine and through its translated script, and compares every leaf. Rows compare as a set unless the leaf's operator declares an order, since the engine runs most operators on several workers. Every plan reads the shared table, written at run time as the CSV the CSV source reads, once whole and once with a hole in every column. The plans are aimed at how fragments go wrong together: a chain, a cast to text, a Union of two halves that share no row, both branches of a Split, a holed integer column, a cast date, and a column named like a placeholder. File Scan reads the shared table's own file from the path Text Input names.TranslatedPlanRunSpeckeeps the two the engine has nothing to compare with: two charts in one plan, and a chart that cannot be drawn.amber's tests take the compiling service's and workflow-operator's classes and test resources, not the modules themselves. The compiling service brings dropwizard 4, which would evict the dropwizard 1 amber is built on.
Runs whose reason is that both paths raise are no longer skipped. They sit in their own list,
variantsBothRefuse, run like any other, and pass only when the engine and the script both refuse, since a script that answers where the engine refuses is as wrong as a different answer. Only a run named there may pass on two errors, so a broken environment cannot turn the suite green. The one exception found is the dummy classifier on an empty table: it fits on nothing into a model that cannot predict, so it stays invariantsNotRunwith that reason. File Scan reads the canonical table's own JSON, which both paths refuse as an archive and as a typed line, so those variants are listed there too. Its archives and typed lines are compared on real files, in the scenarios #8364 and #8363 add. A large binary is written to object storage, which the harness does not give the executor, so that variant is invariantsNotRun.Any related issues, documentation, discussions?
Part of #8325, 23 of 27; that issue lists the set in order. It does not compile until #8357, #8358, #8359, #8509, #8363 and #8388 land. It calls
PyOpExecHarnessfrom #8357,StandaloneRunnerfrom #8358,Comparatorfrom #8359, the two chart comparators from #8509,CuratedHandlersfrom #8363, andConfigGenerator.generateVariantsfrom #8388, which builds on #8361. The edge values pass only once #8341, #8704, #8357, #8709, #8710 and #8597 are in. Each of those fixes how one path reads or writes them. All of them come earlier in the set or merge before it. An empty table runs on every operator once #8766 is in, which hands a port that carried no rows its declared columns.Closes #8425, the task this change is the whole of.
The workflow-level comparison answers the review that asked for plans run end to end. It calls
runTranslatedfrom #8358.Reading the shared table across a whole plan found two engine defects that the per-operator runs cannot reach. #9074, where result storage moved a timestamp before 1883, is fixed by #9076, which these plans need. #9077, where a Python operator pauses the run on a LONG column holding a null and a value past 2^53, is still open, so the chain projects before its sort until it is fixed.
How was this PR tested?
The tests in this change cover it. The whole set is exercised together once the last piece lands: every operator run through the engine and through its generated script, and the two answers compared.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5, Claude Opus 5.5)