fix(visualization): retarget attributeTypeRules at the properties they name - #7249
Open
kz930 wants to merge 2 commits into
Open
fix(visualization): retarget attributeTypeRules at the properties they name#7249kz930 wants to merge 2 commits into
kz930 wants to merge 2 commits into
Conversation
…y name LineConfig keyed its rule by the Scala field names and ScatterMatrixChart by "value" where the property is "Selected Attributes", so the property editor looked up a property that does not exist and skipped the check. Scatter3d and FunnelPlot constrained a "title" neither operator declares, with a bare string rather than an object, so those rules could not have applied under any key; they are removed rather than invented anew. Closes apache#7210 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
👋 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. |
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7249 +/- ##
=========================================
Coverage 82.64% 82.64%
+ Complexity 4080 4079 -1
=========================================
Files 1162 1162
Lines 46282 46282
Branches 5160 5160
=========================================
Hits 38250 38250
+ Misses 6341 6340 -1
- Partials 1691 1692 +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:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🟢 | bs=10 sw=10 sl=64 | 391 | 0.239 | 25,391/33,133/33,133 us | 🟢 -7.9% / 🔴 +115.3% |
| 🔴 | bs=100 sw=10 sl=64 | 795 | 0.485 | 125,568/138,268/138,268 us | 🔴 +5.3% / 🔴 +30.6% |
| 🔴 | bs=1000 sw=10 sl=64 | 909 | 0.555 | 1,092,661/1,223,977/1,223,977 us | 🔴 +8.9% / 🔴 +20.4% |
Baseline details
Latest main 436b37e from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 391 tuples/sec | 408 tuples/sec | 790.88 tuples/sec | -4.2% | -50.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.249 MB/s | 0.483 MB/s | -4.0% | -50.5% |
| bs=10 sw=10 sl=64 | p50 | 25,391 us | 25,043 us | 12,348 us | +1.4% | +105.6% |
| bs=10 sw=10 sl=64 | p95 | 33,133 us | 35,993 us | 15,390 us | -7.9% | +115.3% |
| bs=10 sw=10 sl=64 | p99 | 33,133 us | 35,993 us | 18,935 us | -7.9% | +75.0% |
| bs=100 sw=10 sl=64 | throughput | 795 tuples/sec | 828 tuples/sec | 1,009 tuples/sec | -4.0% | -21.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.485 MB/s | 0.505 MB/s | 0.616 MB/s | -4.0% | -21.2% |
| bs=100 sw=10 sl=64 | p50 | 125,568 us | 119,243 us | 99,753 us | +5.3% | +25.9% |
| bs=100 sw=10 sl=64 | p95 | 138,268 us | 141,755 us | 105,895 us | -2.5% | +30.6% |
| bs=100 sw=10 sl=64 | p99 | 138,268 us | 141,755 us | 113,263 us | -2.5% | +22.1% |
| bs=1000 sw=10 sl=64 | throughput | 909 tuples/sec | 913 tuples/sec | 1,037 tuples/sec | -0.4% | -12.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.557 MB/s | 0.633 MB/s | -0.4% | -12.3% |
| bs=1000 sw=10 sl=64 | p50 | 1,092,661 us | 1,094,716 us | 976,194 us | -0.2% | +11.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,223,977 us | 1,124,462 us | 1,016,411 us | +8.9% | +20.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,223,977 us | 1,124,462 us | 1,045,212 us | +8.9% | +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,511.38,200,128000,391,0.239,25391.01,33133.40,33133.40
1,100,10,64,20,2514.76,2000,1280000,795,0.485,125568.31,138267.75,138267.75
2,1000,10,64,20,21996.28,20000,12800000,909,0.555,1092661.33,1223977.14,1223977.14The Required Checks summary read a cancelled precheck from a superseded run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Four
attributeTypeRulesname keys that match no property, so the property editor'sfindAttributeTypereturnsundefinedandcheckConstraintreturns without checking anything.LineConfigused the Scala field names rather than the@JsonPropertynames, andScatterMatrixChartOpDescnamedvaluewhere the property isSelected Attributes; both are retargeted at the property they meant.Scatter3dChartOpDescandFunnelPlotOpDescconstrained atitlethat neither operator declares, and did so with the bare string"string"rather than an object, so even under a correct keyconstraint.enum/const/allOfwould all be undefined and the check would still no-op; those two rules are removed rather than invented anew.Why are the changes needed?
A line chart's x and y axes accept string columns today despite declaring
["integer", "long", "double"], and the same holds for the scatter matrix's dimensions.BandConfigextendsLineConfigand inherited the same dead rule. Nothing reports a key that names no property, so the rules read as enforced while enforcing nothing.Does this PR introduce any user-facing change?
Yes. Selecting a non-numeric column for a line chart's axes or the scatter matrix's Selected Attributes now shows the type warning the rule always intended. Nothing changes for a numeric column, and removing the two
titlerules changes nothing at all, since they never applied.How was this patch tested?
WorkflowOperator/compile,WorkflowOperator/scalafmtCheckAll, and the four operators' existing descriptor specs (24 tests, all passing).Closes #7210
Generated-by: Claude Code (Claude Opus 5)