Skip to content

feat(frontend): check the attribute type rule on every column a property names - #7980

Open
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:fix-multi-column-attribute-type-rules
Open

feat(frontend): check the attribute type rule on every column a property names#7980
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:fix-multi-column-attribute-type-rules

Conversation

@kz930

@kz930 kz930 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

An operator property that takes several column names never had its attributeTypeRules checked. The checkAttributeType validator reads the property's value as a single attribute name and asks the compiling service for that name's type. A property holding a list handed it an array, no attribute matched, the type came back undefined, and the validator treated the property as not yet filled in and moved on. Single-column properties were unaffected, which is why the gap went unnoticed.

Four operators on main declare a rule on such a property, so all four were inert: Scatter Matrix Chart's Selected Attributes, Parallel Coordinates Plot's dimensions, Radar Chart's valueColumns, and Radar Plot's selectedAttributes.

The validator now reads both shapes as a list of names and checks each one on its own. The warning names the column that broke the rule rather than the property, which matters when a property holds several. Assembling that message inside the loop also removes the duplicated type lookup the outer catch block was doing, along with the note it left asking for exactly that.

The const $data comparison still resolves a single name. Both rules that use it, Hash Join's and Interval Join's, name a single-column property.

With the rule now reaching every named column, this also declares the one the sklearn advanced trainers were missing. Their Selected Features columns are passed to fit with no preprocessing, so a text or binary column fails inside scikit-learn with "could not convert string to float". Every other Texera attribute type is fitted, a timestamp included: it arrives as datetime64 and is read as epoch microseconds. Declaring this rule before the fix above would have had no effect.

Any related issues, documentation, discussions?

Closes #7979

How was this PR tested?

Three cases were added to the existing attributeTypeRules suite in operator-property-edit-frame.component.spec.ts: a multi-column property whose columns all match, one where the second column violates the rule and the warning names that column, and one that names no columns at all. The suite passes with 222 tests.

One case was added to SklearnAdvancedBaseDescSpec, reading the accepted set out of the generated operator schema. That suite passes with 8 tests.

The accepted set itself was checked against scikit-learn rather than written from memory. Each Texera attribute type was fitted through KNeighborsClassifier, KNeighborsRegressor, SVC and SVR. Only string and binary raise.

It was also checked by hand in the running application, on a workflow feeding one CSV source into both a Bar Chart and a Scatter Matrix Chart. Before the change, choosing a string column as the Bar Chart's Value Column warned and choosing the same column under the Scatter Matrix's Selected Attributes did not. After it, both warn. Selecting a numeric column and a string one together names the string one in the warning, not the property and not the column that was fine.

One pre-existing behaviour is worth noting for a reviewer reproducing this: the warning appears once the field has been edited, not on reloading a workflow whose stored value already violates the rule. That is the same before and after, and the same for single-column properties.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

…rty names

The checkAttributeType validator read a property's value as a single
attribute name, so a property holding a list of names resolved to no type
at all and the rule was skipped without a word. Four operators on main
declare such a rule and none of them warned: Scatter Matrix Chart's
Selected Attributes, Parallel Coordinates Plot's dimensions, Radar
Chart's valueColumns and Radar Plot's selectedAttributes.

Now that a rule reaches every column a property names, declare the one
the sklearn advanced trainers were missing. Their Selected Features
columns go into fit untouched, so each has to be one scikit-learn reads
as a number, which a timestamp is: it arrives as datetime64 and is fitted
as epoch microseconds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI common labels Aug 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @carloea2, @Ma77Ball, @mengw15
    You can notify them by mentioning @carloea2, @Ma77Ball, @mengw15 in a comment.

@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.12%. Comparing base (9f2102e) to head (3854429).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...it-frame/operator-property-edit-frame.component.ts 95.23% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7980      +/-   ##
============================================
+ Coverage     92.89%   93.12%   +0.23%     
- Complexity     4633     4634       +1     
============================================
  Files          1174     1174              
  Lines         47442    47453      +11     
  Branches       5304     5308       +4     
============================================
+ Hits          44070    44192     +122     
+ Misses         1868     1785      -83     
+ Partials       1504     1476      -28     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø)
agent-service 99.32% <ø> (ø) Carriedforward from 9f2102e
amber 89.21% <ø> (+<0.01%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø)
config-service 86.73% <ø> (ø)
file-service 84.48% <ø> (ø)
frontend 95.63% <95.23%> (+0.53%) ⬆️
notebook-migration-service 79.31% <ø> (+0.17%) ⬆️
pyamber 97.52% <ø> (ø) Carriedforward from 9f2102e
workflow-compiling-service 77.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 3 worse · ⚪ 10 noise (<±5%) · 0 without baseline

Compared against main f236751 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 395 0.241 24,413/33,860/33,860 us 🟢 -15.3% / 🔴 +148.9%
bs=100 sw=10 sl=64 820 0.501 119,206/148,325/148,325 us ⚪ within ±5% / 🔴 +61.9%
bs=1000 sw=10 sl=64 924 0.564 1,085,338/1,129,817/1,129,817 us ⚪ within ±5% / 🔴 +29.6%
Baseline details

Latest main f236751 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 395 tuples/sec 422 tuples/sec 908.02 tuples/sec -6.4% -56.5%
bs=10 sw=10 sl=64 MB/s 0.241 MB/s 0.258 MB/s 0.554 MB/s -6.6% -56.5%
bs=10 sw=10 sl=64 p50 24,413 us 22,073 us 10,907 us +10.6% +123.8%
bs=10 sw=10 sl=64 p95 33,860 us 39,993 us 13,604 us -15.3% +148.9%
bs=10 sw=10 sl=64 p99 33,860 us 39,993 us 16,697 us -15.3% +102.8%
bs=100 sw=10 sl=64 throughput 820 tuples/sec 858 tuples/sec 1,188 tuples/sec -4.4% -31.0%
bs=100 sw=10 sl=64 MB/s 0.501 MB/s 0.523 MB/s 0.725 MB/s -4.2% -30.9%
bs=100 sw=10 sl=64 p50 119,206 us 114,255 us 85,003 us +4.3% +40.2%
bs=100 sw=10 sl=64 p95 148,325 us 146,334 us 91,599 us +1.4% +61.9%
bs=100 sw=10 sl=64 p99 148,325 us 146,334 us 99,202 us +1.4% +49.5%
bs=1000 sw=10 sl=64 throughput 924 tuples/sec 943 tuples/sec 1,221 tuples/sec -2.0% -24.3%
bs=1000 sw=10 sl=64 MB/s 0.564 MB/s 0.576 MB/s 0.745 MB/s -2.1% -24.3%
bs=1000 sw=10 sl=64 p50 1,085,338 us 1,069,081 us 837,593 us +1.5% +29.6%
bs=1000 sw=10 sl=64 p95 1,129,817 us 1,105,273 us 881,015 us +2.2% +28.2%
bs=1000 sw=10 sl=64 p99 1,129,817 us 1,105,273 us 915,550 us +2.2% +23.4%
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,506.84,200,128000,395,0.241,24412.61,33860.20,33860.20
1,100,10,64,20,2438.02,2000,1280000,820,0.501,119206.05,148324.51,148324.51
2,1000,10,64,20,21640.14,20000,12800000,924,0.564,1085337.50,1129816.81,1129816.81

@kz930

kz930 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan @carloea2

1 similar comment
@kz930

kz930 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan @carloea2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common fix frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

attributeTypeRules is never checked on a property that takes several columns

2 participants