feat: add persisted top-K categorical lumping - #2596
Open
ranadeepsingh wants to merge 4 commits into
Open
Conversation
|
Hey @ranadeepsingh 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new core Spark ML stage to address high-cardinality categorical features by learning per-column top‑K string values at fit time and applying deterministic lumping at transform time, with full persistence/serialization support consistent with SynapseML’s Estimator/Model patterns.
Changes:
- Introduces
LumpFeatures(Estimator) andLumpFeaturesModel(Model) with multi-columnlumpRules, deterministic top‑K selection, configurable null handling, andotherValuecollision validation. - Implements persisted learned state via
keptValuesJsonand supports load/save, copy, and schema transformation behavior. - Adds a comprehensive Scala test suite covering ranking, null handling, collisions, schema/metadata behavior, special column names, and persistence round-trips.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/scala/com/microsoft/azure/synapse/ml/stages/LumpFeatures.scala | New Estimator/Model implementation for persisted top‑K categorical lumping with deterministic ranking, schema validation, and serialization. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/stages/LumpFeaturesSuite.scala | New unit tests validating behavior, edge cases, schema expectations, and persistence round-trip. |
Review details
Suppressed comments (1)
core/src/main/scala/com/microsoft/azure/synapse/ml/stages/LumpFeatures.scala:227
handleNull='keep'currently only preserves nulls when the input schema marks the column as nullable; otherwise the expression coalesces nulls tootherValue. This makes runtime behavior depend on schema nullability and can violate the documented semantics that'keep' preserves null. RefactorlumpExprto preserve nulls wheneverkeepNullsis true, independent of schema nullability, and drop theinputNullableparameter.
transformSchema(dataset.schema)
val other = getOtherValue
val keepNulls = getHandleNull == "keep"
getKeptValues.foldLeft(dataset.toDF()) { case (acc, (name, values)) =>
acc.withColumn(name, lumpExpr(name, values, other, keepNulls, acc.schema(name).nullable))
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| val keepNulls = getHandleNull == "keep" | ||
| val fields = schema.fields.map { f => | ||
| if (ruleCols.contains(f.name)) { | ||
| StructField(f.name, StringType, nullable = f.nullable && keepNulls, Metadata.empty) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2596 +/- ##
==========================================
+ Coverage 83.60% 84.80% +1.20%
==========================================
Files 334 336 +2
Lines 17806 18075 +269
Branches 1623 1633 +10
==========================================
+ Hits 14887 15329 +442
+ Misses 2919 2746 -173 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ranadeepsingh
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Aug 1, 2026
## Summary Add dedicated transformer fuzzing coverage for LumpFeaturesModel so global experiment, serialization, Python, and R coverage gates recognize the persisted model. ## Prompting Intent Repair the concrete UnitTests core failure from PR microsoft#2596 after Azure build 229219360 reported that LumpFeaturesModel had no directly registered fuzzers, while preserving all estimator tests. ## Linked Sources - Pull request: microsoft#2596 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229219360 - Original proposal: microsoft#1941 - Feature request: microsoft#1891 ## Rationale Register a real TransformerFuzzing test object instead of exempting the model. This exercises deterministic transforms and model persistence while generating Python and R correspondence coverage expected by the repository-wide FuzzingTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
ranadeepsingh
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Aug 1, 2026
## Summary Persist each fitted top-K alongside retained values and reject incompatible LumpFeaturesModel lumpRules mutations across direct setters, generic generated-binding transfer, copy overrides, and loaded models. ## Prompting Intent Address the independent medium-severity API review finding on PR microsoft#2596 without removing API-compatible params. Ensure a fitted model can never silently score with learned values that disagree with a post-fit K, and cover persistence, copy, Scala, Java, JSON, and generated-binding paths. ## Linked Sources - Pull request: microsoft#2596 - Original proposal: microsoft#1941 - Feature request: microsoft#1891 - Independent review finding supplied in the PR follow-up request - No Azure DevOps work item was supplied; tracking is through the linked GitHub issue. ## Rationale Encode the fitted top-K inside the existing model-only keptValuesJson state instead of adding another generated mutable parameter. Direct model setters fail immediately, while transform-time state validation protects generic Param paths used by generated bindings. Exact no-op rule assignment remains allowed, and incompatible copy overrides fail before returning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
Addressed the independent API review finding in 5443214:
Validation: 24 estimator tests, 4 model tests, 10 repository fuzzing checks, core compile/scalastyle, codegen, and generated-wrapper @copilot-pull-request-reviewer please re-review the updated head. |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
|
Follow-up API fix is now fully green on head
Re-review requested for the fitted |
## Summary Add LumpFeatures as a Spark ML estimator with a persisted model, deterministic top-K learning, explicit other-bucket and null semantics, schema-safe transforms, generated bindings, and comprehensive tests. ## Prompting Intent Recreate the valuable proposal from GitHub PR microsoft#1941 for current SynapseML without fitting during transform. Preserve lumpRules compatibility while covering persistence, copy behavior, special column names, unseen values, collisions, and Scala-first Python code generation. ## Linked Sources - Original proposal PR: microsoft#1941 - Feature request: microsoft#1891 - No Azure DevOps work item was supplied; tracking is through the linked GitHub issue. ## Rationale Learn category frequencies once in fit and persist only retained values so scoring is stable and side-effect free. Restrict v1 to string columns, rank ties by value, preserve nulls by default, and reject other-bucket collisions rather than silently merging real categories. Use Spark SQL expressions instead of UDFs and retain the multi-column lumpRules API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Add dedicated transformer fuzzing coverage for LumpFeaturesModel so global experiment, serialization, Python, and R coverage gates recognize the persisted model. ## Prompting Intent Repair the concrete UnitTests core failure from PR microsoft#2596 after Azure build 229219360 reported that LumpFeaturesModel had no directly registered fuzzers, while preserving all estimator tests. ## Linked Sources - Pull request: microsoft#2596 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229219360 - Original proposal: microsoft#1941 - Feature request: microsoft#1891 ## Rationale Register a real TransformerFuzzing test object instead of exempting the model. This exercises deterministic transforms and model persistence while generating Python and R correspondence coverage expected by the repository-wide FuzzingTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Persist each fitted top-K alongside retained values and reject incompatible LumpFeaturesModel lumpRules mutations across direct setters, generic generated-binding transfer, copy overrides, and loaded models. ## Prompting Intent Address the independent medium-severity API review finding on PR microsoft#2596 without removing API-compatible params. Ensure a fitted model can never silently score with learned values that disagree with a post-fit K, and cover persistence, copy, Scala, Java, JSON, and generated-binding paths. ## Linked Sources - Pull request: microsoft#2596 - Original proposal: microsoft#1941 - Feature request: microsoft#1891 - Independent review finding supplied in the PR follow-up request - No Azure DevOps work item was supplied; tracking is through the linked GitHub issue. ## Rationale Encode the fitted top-K inside the existing model-only keptValuesJson state instead of adding another generated mutable parameter. Direct model setters fail immediately, while transform-time state validation protects generic Param paths used by generated bindings. Exact no-op rule assignment remains allowed, and incompatible copy overrides fail before returning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rework the persisted top-K lumping stage so it is dependable as a training feature-engineering step, not just correct on a single-column happy path. Algorithm - Add minCount and minFreq eligibility filters, applied before the lumpRules top-K cap. This matches the order scikit-learn's OneHotEncoder uses for min_frequency and max_categories, and R forcats / feature-engine use the same primary-threshold + secondary-cap shape. Top-K alone is blind to the distribution: it lumps healthy levels in a low-cardinality column, and in a long-tailed column it retains values covering almost none of the rows. Both default to no-ops, so existing behaviour is unchanged. - Rewrite fit as a single pass. The rule columns are melted into (column, value) pairs and ranked with one windowed aggregation instead of one full scan per column plus a separate collision scan. Besides cutting fit from N+1 Spark jobs to 1, this guarantees every column is learned from the same materialization of the input; per-column jobs silently learn from different rows when the upstream plan is non-deterministic (sample, rand, unordered limit). API - Add an optional outputCols map so lumped values can be written to new columns instead of destroying the raw ones. Unset means in-place, so the default is unchanged. Destinations are validated (known source, non-empty, distinct, not already in the input schema) and ordered deterministically so transformSchema always matches transform. - Fix the nullability contract: the declared schema now derives nullability from handleNull alone instead of intersecting it with the input column's nullability, so a non-nullable input under handleNull='keep' no longer declares a non-nullable output that the expression may not honour. This is the reviewer comment on the PR; the transform expression uses a typed null literal so declared and actual schemas agree on non-nullable inputs too. - Expose the learned values to Python. LumpFeaturesModel becomes an internal wrapper with a hand-written override providing getKeptValues(), backed by a new getKeptValuesAsJson on the Scala model, so a fit can be audited from Python instead of being an opaque JSON param. Docs and tests - Document LumpFeatures in docs/Quick Examples with runnable Python and Scala examples; the stage was previously absent from the stages doc table. - Add 13 tests covering the frequency filters and their ordering against the cap, per-column denominators, joint vs single-column fit agreement, outputCols behaviour/validation/round-trip, the non-nullable schema contract, and the JSON accessor. 41 tests pass, scalastyle clean on main and test, codegen and black clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a108ab7-6879-4fa6-81de-ef2d43eb3ec5
ranadeepsingh
force-pushed
the
copilot/ancient-1941-lump-features
branch
from
August 4, 2026 18:33
5443214 to
a676184
Compare
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
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
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.
Summary
Recreates the valuable categorical-lumping proposal from #1941 for the current SynapseML codebase and addresses #1891.
LumpFeaturesas a Spark MLEstimatorplus persistedLumpFeaturesModellumpRulesAPI, including Scala map, Java map, legacy JSON, and generated Pythondictbindingsfit;transformnever refitsotherValue, and makes null handling configurablecountcolumn names safelyThis is a new implementation; it does not alter the ancient PR branch.
Validation
core/compilecore/testOnly com.microsoft.azure.synapse.ml.stages.LumpFeaturesSuite— 18 tests passedcore/scalastyle; core/Test/scalastylecodegenLumpFeatures.pyandLumpFeaturesModel.pysyntax-compiled withpy_compileDesign notes
The initial API intentionally supports string columns only so the explicit other bucket cannot silently change numeric or boolean column types. Nulls are excluded from frequency counts and preserved by default; unseen non-null values always use the other bucket. Fitting fails if
otherValuealready occurs in any configured column, avoiding ambiguous category merges.Related: #1941, #1891