You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@texera.apache.org by "kz930 (via GitHub)" <gi...@apache.org> on 2026/07/22 21:50:51 UTC

[PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

kz930 opened a new pull request, #6805:
URL: https://github.com/apache/texera/pull/6805

   ### What changes were proposed in this PR?
   
   Fixes a double-encoding bug in `MachineLearningScorerOpDesc` where the selected metrics collapse into a single malformed `metric_list` element.
   
   `getSelectedMetrics()` returns the chosen metric names as one comma-separated, already-quoted fragment (e.g. `'Accuracy','F1 Score'`) and is spliced into the generated Python as a list:
   
   ```
   metric_list = [${getSelectedMetrics()}]
   ```
   
   Its return type was **`EncodableString`**, so the Python template builder **re-encoded the whole fragment as one Python string value** instead of splicing it verbatim. The emitted code became:
   
   ```python
   metric_list = [self.decode_python_template('J0FjY3VyYWN5JywnRjEgU2NvcmUn')]
   ```
   
   where the base64 `J0FjY3VyYWN5JywnRjEgU2NvcmUn` decodes to `'Accuracy','F1 Score'` — i.e. the entire quoted list collapses into **one** decoded string element:
   
   ```python
   metric_list = ["'Accuracy','F1 Score'"]   # ONE element, incl. inner quotes
   ```
   
   instead of the intended:
   
   ```python
   metric_list = ['Accuracy', 'F1 Score']
   ```
   
   So every downstream `if 'X' in metric_list` and `for metric in metric_list` / `metrics_func[metric]` operates on that single malformed element — the selected metrics are never matched, and the classification path hits a `KeyError` on the bogus key. The Scorer produces wrong/empty output for both the classification and regression paths.
   
   **Fix:** change `getSelectedMetrics()`'s return type from `EncodableString` to plain `String`, so the builder splices it verbatim into `metric_list = ['Accuracy','F1 Score']`. The method body is unchanged.
   
   ```diff
   -  private def getSelectedMetrics(): EncodableString = {
   +  private def getSelectedMetrics(): String = {
        val metric = if (isRegression) regressionMetrics else classificationMetrics
        metric.map(metric => getMetricName(metric)).mkString("'", "','", "'")
      }
   ```
   
   ### Any related issues, documentation, discussions?
   
   Closes #6790
   
   ### How was this PR tested?
   
   Added a regression test to `MachineLearningScorerOpDescSpec` that constructs the descriptor with `classificationMetrics = List(accuracy, f1Score)`, calls `generatePythonCode()`, and asserts:
   - the emitted code contains `metric_list = ['Accuracy','F1 Score']` (verbatim), and
   - the metric names are **not** base64-re-encoded through the template builder.
   
   Both assertions fail on `main` (the line is emitted as `metric_list = [self.decode_python_template('J0FjY3VyYWN5JywnRjEgU2NvcmUn')]`) and pass with this change.
   
   Ran the full suite locally:
   
   ```
   sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.machineLearning.Scorer.MachineLearningScorerOpDescSpec"
   ...
   Tests: succeeded 7, failed 0, canceled 0, ignored 0, pending 0
   All tests passed.
   ```
   
   ### Was this PR authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Code (Claude Opus 4.8)
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "kz930 (via GitHub)" <gi...@apache.org>.

kz930 commented on code in PR #6805:
URL: https://github.com/apache/texera/pull/6805#discussion_r3634691828


##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/Scorer/MachineLearningScorerOpDesc.scala:
##########
@@ -122,7 +122,10 @@ class MachineLearningScorerOpDesc extends PythonOperatorDescriptor {
       case _                           => throw new IllegalArgumentException("Unknown metric type")
     }
 
-  private def getSelectedMetrics(): EncodableString = {
+  // Must be a plain String, not EncodableString: this is a raw Python fragment
+  // spliced verbatim into `metric_list = [...]`. An EncodableString would be
+  // re-encoded as one quoted value, collapsing the list into a single element.
+  private def getSelectedMetrics(): String = {
     // Return a string of metrics using the getEachScorerName() method

Review Comment:
   Good catch — fixed in c89654932. The comment referenced a nonexistent `getEachScorerName()`; the code uses `getMetricName`. Since the line only restated the code below it, I removed the stale comment rather than reword it (the "why" comment above `getSelectedMetrics` about needing a plain String stays).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #6805:
URL: https://github.com/apache/texera/pull/6805#discussion_r3634623652


##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/Scorer/MachineLearningScorerOpDesc.scala:
##########
@@ -122,7 +122,10 @@ class MachineLearningScorerOpDesc extends PythonOperatorDescriptor {
       case _                           => throw new IllegalArgumentException("Unknown metric type")
     }
 
-  private def getSelectedMetrics(): EncodableString = {
+  // Must be a plain String, not EncodableString: this is a raw Python fragment
+  // spliced verbatim into `metric_list = [...]`. An EncodableString would be
+  // re-encoded as one quoted value, collapsing the list into a single element.
+  private def getSelectedMetrics(): String = {
     // Return a string of metrics using the getEachScorerName() method

Review Comment:
   The inline comment mentions `getEachScorerName()`, but this method doesn’t exist here (the implementation uses `getMetricName`). This is misleading documentation in a newly-touched block; update or remove the comment so it matches the current logic.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.

aglinxinyuan merged PR #6805:
URL: https://github.com/apache/texera/pull/6805


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "kz930 (via GitHub)" <gi...@apache.org>.

kz930 commented on PR #6805:
URL: https://github.com/apache/texera/pull/6805#issuecomment-5052473784

   @carloea2, @aglinxinyuan May any of you take a look at this please?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "kz930 (via GitHub)" <gi...@apache.org>.

kz930 commented on code in PR #6805:
URL: https://github.com/apache/texera/pull/6805#discussion_r3634691828


##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/Scorer/MachineLearningScorerOpDesc.scala:
##########
@@ -122,7 +122,10 @@ class MachineLearningScorerOpDesc extends PythonOperatorDescriptor {
       case _                           => throw new IllegalArgumentException("Unknown metric type")
     }
 
-  private def getSelectedMetrics(): EncodableString = {
+  // Must be a plain String, not EncodableString: this is a raw Python fragment
+  // spliced verbatim into `metric_list = [...]`. An EncodableString would be
+  // re-encoded as one quoted value, collapsing the list into a single element.
+  private def getSelectedMetrics(): String = {
     // Return a string of metrics using the getEachScorerName() method

Review Comment:
   Good catch — fixed in c89654932. The comment referenced a nonexistent `getEachScorerName()`; the code uses `getMetricName`. Since the line only restated the code below it, I removed the stale comment rather than reword it (the "why" comment above `getSelectedMetrics` about needing a plain String stays).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #6805:
URL: https://github.com/apache/texera/pull/6805#issuecomment-5052072958

   <!-- texera-benchmarks-comment -->
   ## ⚠️ Benchmark changes need a look
   
   🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline
   
   > Compared against main `bf1f589` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/29960978967)
   
   | | config | throughput | MB/s | latency | max Δ latest / 7d |
   |---|---|---:|---:|---:|---:|
   | 🔴 | bs=10 sw=10 sl=64 | 399 | 0.244 | 25,561/30,365/30,365 us | 🔴 +16.1% / 🔴 +100.0% |
   | 🔴 | bs=100 sw=10 sl=64 | 816 | 0.498 | 119,201/148,560/148,560 us | 🔴 +7.4% / 🔴 +38.3% |
   | ⚪ | bs=1000 sw=10 sl=64 | 946 | 0.578 | 1,057,099/1,112,054/1,112,054 us | ⚪ within ±5% / 🔴 +8.0% |
   
   <details><summary>Baseline details</summary>
   
   Latest main `bf1f589` from same runner
   
   | config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
   |---|---|---:|---:|---:|---:|---:|
   | bs=10 sw=10 sl=64 | throughput | 399 tuples/sec | 439 tuples/sec | 760.2 tuples/sec | -9.1% | -47.5% |
   | bs=10 sw=10 sl=64 | MB/s | 0.244 MB/s | 0.268 MB/s | 0.464 MB/s | -9.0% | -47.4% |
   | bs=10 sw=10 sl=64 | p50 | 25,561 us | 22,022 us | 12,781 us | +16.1% | +100.0% |
   | bs=10 sw=10 sl=64 | p95 | 30,365 us | 33,568 us | 16,118 us | -9.5% | +88.4% |
   | bs=10 sw=10 sl=64 | p99 | 30,365 us | 33,568 us | 19,074 us | -9.5% | +59.2% |
   | bs=100 sw=10 sl=64 | throughput | 816 tuples/sec | 851 tuples/sec | 988.68 tuples/sec | -4.1% | -17.5% |
   | bs=100 sw=10 sl=64 | MB/s | 0.498 MB/s | 0.519 MB/s | 0.603 MB/s | -4.0% | -17.5% |
   | bs=100 sw=10 sl=64 | p50 | 119,201 us | 117,092 us | 101,448 us | +1.8% | +17.5% |
   | bs=100 sw=10 sl=64 | p95 | 148,560 us | 138,316 us | 107,410 us | +7.4% | +38.3% |
   | bs=100 sw=10 sl=64 | p99 | 148,560 us | 138,316 us | 120,258 us | +7.4% | +23.5% |
   | bs=1000 sw=10 sl=64 | throughput | 946 tuples/sec | 941 tuples/sec | 1,023 tuples/sec | +0.5% | -7.5% |
   | bs=1000 sw=10 sl=64 | MB/s | 0.578 MB/s | 0.575 MB/s | 0.624 MB/s | +0.5% | -7.4% |
   | bs=1000 sw=10 sl=64 | p50 | 1,057,099 us | 1,060,123 us | 983,888 us | -0.3% | +7.4% |
   | bs=1000 sw=10 sl=64 | p95 | 1,112,054 us | 1,108,319 us | 1,030,100 us | +0.3% | +8.0% |
   | bs=1000 sw=10 sl=64 | p99 | 1,112,054 us | 1,108,319 us | 1,056,513 us | +0.3% | +5.3% |
   
   </details>
   
   <details><summary>Raw CSV</summary>
   
   ```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,501.07,200,128000,399,0.244,25560.82,30364.86,30364.86
   1,100,10,64,20,2451.76,2000,1280000,816,0.498,119200.51,148560.07,148560.07
   2,1000,10,64,20,21134.11,20000,12800000,946,0.578,1057099.13,1112054.18,1112054.18
   ```
   
   </details>
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #6805:
URL: https://github.com/apache/texera/pull/6805#issuecomment-5051986139

   <!-- texera-reviewer-suggestion -->
   ### Automated Reviewer Suggestions
   
   Based on the `git blame` history of the changed files, we recommend the following reviewers:
   
   - **Contributors with relevant context:** `@carloea2`, `@aglinxinyuan`
     You can notify them by mentioning `@carloea2`, `@aglinxinyuan` in a comment.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(MachineLearningScorer): splice metric_list verbatim instead of double-encoding [texera]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #6805:
URL: https://github.com/apache/texera/pull/6805#issuecomment-5052008056

   ### :warning: JUnit XML file not found
   The CLI was unable to find any JUnit XML files to upload.
   For more help, visit our [troubleshooting guide](https://docs.codecov.com/docs/test-analytics#troubleshooting).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org