Repository navigation
fix: preserve VW Generic input schemas - #2821
Rana Singh (ranadeepsingh) merged 2 commits into
Conversation
Keep all input fields and append the native prediction schema, restoring top-level metadata after row encoding. Reject prediction-name collisions using the Spark session case-sensitivity setting. Cover schema ordering, scalar and array predictions, custom inputs and collision behavior.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Lenin Mookiah (@leninworld) 👋! 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. |
There was a problem hiding this comment.
🟡 Changes recommended
Rebuilding nested structs drops child-field metadata, including within arrays and maps, causing output schemas to differ from transformSchema and requiring a metadata-preserving fix with full-schema regression coverage.
1 open issue
What changed in this PR
Addresses #2817 by aligning VowpalWabbitGenericModel’s declared output schema with its emitted rows, preserving input columns before appending flat prediction fields.
Changes:
- Preserve input-column order, types and nullability, and restore top-level metadata after encoding.
- Reject non-string example columns and prediction-name collisions using Spark’s case-sensitivity setting.
- Add regressions for scalar and array predictions, reordered and custom inputs, metadata, and collision handling.
| File | Description |
|---|---|
vw/src/test/scala/com/microsoft/azure/synapse/ml/vw/VerifyVowpalWabbitGeneric.scala |
Adds schema-preservation and collision regressions, plus multi-column fuzzing coverage. |
vw/src/main/scala/com/microsoft/azure/synapse/ml/vw/VowpalWabbitGeneric.scala |
Preserves input schemas, restores top-level metadata, and validates input types and prediction-name conflicts. |
Review effort: Balanced
## Summary Restore each output column's declared datatype after Spark row encoding so nested field metadata survives VowpalWabbitGenericModel.transform. Add public Spark regressions for nested schemas and categorical label decoding. ## Prompting Intent Apply the independently reproduced nested-metadata correction to the existing external contribution, preserve the contributor's implementation and history, and validate the improvement through the PR and external-contributor loops. ## Linked Sources - Original report: microsoft#2817 - Contributor PR: microsoft#2821 - Review finding: microsoft#2821 (comment) - Spark row encoder: https://github.com/apache/spark/blob/v3.5.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala - Spark serializer: https://github.com/apache/spark/blob/v3.5.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SerializerBuildHelper.scala - Spark casts: https://github.com/apache/spark/blob/v3.5.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala - Label decoder: https://github.com/apache/spark/blob/v3.5.0/mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala ## Rationale Top-level aliases cannot recover metadata on nested struct fields erased by ExpressionEncoder. Casting to the original datatype restores that schema while retaining the existing top-level metadata alias. Spark treats structurally identical casts as representation-preserving, avoiding recursive reconstruction, new native calls, or shuffles. Public tests check structs, arrays, map keys and values, nullability, collected values, and downstream IndexToString behavior. No public signatures, parameter formats, dependency pins, or CI files change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ed340ce9-385a-41e0-86c4-fe0f09097478
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks Lenin Mookiah (@leninworld) for fixing VW Generic's handling of inputs with multiple columns! I kept your schema-preservation approach and pushed the additive follow-up d24b27872c. It restores nested metadata that Spark's row encoder drops and adds public regressions for nested schema/value preservation and categorical decoding through Azure build 245670108, triggered by The current-head automated review has no open or suppressed findings. GitHub workflow jobs passed; CodeQL's aggregate comparison remains neutral because of an unmatched target-branch scan configuration, as documented in the description. Please review the follow-up and sign off if it fits your intent. If it does not fit your vision for the PR, feel free to revert my commit, or let me know and I will revert it. CLA validation has passed; your sign-off and maintainer approval remain outstanding. |
LGTM |
VW Generic predictions could fail or misalign output fields when input data included IDs, labels, or other columns alongside the VW example. This change preserves the complete input schema and values, including nested metadata needed by downstream Spark ML stages, and appends the existing flat prediction fields.
Related Issues/PRs
Fixes #2817. The source-inspection finding is now backed by public Spark regressions.
What changes are proposed in this pull request?
The contributor's fix keeps every input column in its original order, with its datatype, nullability and metadata. It rejects non-string example columns and names that collide with prediction fields, using the dataset session's Spark case-sensitivity setting.
The additive maintainer follow-up restores nested field metadata that Spark's row encoder drops. A cast to the original datatype, followed by the existing metadata alias, preserves structs, arrays and maps without rebuilding their values. New regressions cover nested nullability and values, plus categorical metadata used by
IndexToString.Single-column prediction values and flat prediction-column names are unchanged. Inputs named like prediction fields now receive an actionable error instead of ambiguous duplicate columns. Input nullability follows the original schema. Public JVM signatures, serialized parameters and dependencies are unchanged.
How is this patch tested?
Maintainer follow-up validation uses JDK 11, Spark 3.5.0, Scala 2.12.17 and VW JNI 9.3.0 in local WSL Spark:
sbt vw/Test/scalastyle "vw/testOnly com.microsoft.azure.synapse.ml.vw.VerifyVowpalWabbitGeneric -- -z nested": style passed; both new regressions failed as intended. The nested schema lost child metadata, and downstreamIndexToStringthrewClassCastException.sbt scalastyle Test/scalastyle compile Test/compile getDatasets vw/test core/codegen vw/codegen: passed. All 118 VW tests passed across 15 suites, with zero failures, cancellations or skips. Local XML reports confirm both nested regressions and the DSJSON/coordinated-learning cases executed.black --check --extend-exclude 'docs/' .with Black 22.3.0: passed, 216 files unchanged.python3 "$validationScript" "$worktree"using the session-localpr2821-python-validation.pyharness and generated core/VW wrappers: passed all six scalar/array x original/copy/reload cases. Checks cover schema and value preservation, nested categorical decoding, prediction parity and verified JVM class provenance from the fixed worktree. The harness is local evidence, not an installed-release or hosted-CI result./azp runwithreason=pullRequest. Its merge commitdbf9b83148d3ce43763398d2dcecfb9b70345f68contains follow-up headd24b27872c2b8bf8be0c8d3715b4ec59894e0954and target6c2699defcfb24d5bff5ba55f2252ba2d8cbea24. All 118 Scala VW tests passed with no skips; Generic suite results explicitly include both new nested-metadata regressions. All 23 Python VW tests passed with no skips, and the VW R job passed.UnitTests formattempt failed during Azure CLI authentication, before tests, because the certificate returned formsdata.visualstudio.comhad only*.azureedge.netas its alternate name. Retrying only that failed job on the same source revision succeeded: 53 tests passed, with two existing explicitignorecases inListCustomModelsSuitereported as not executed. The successful jobs were not rerun, and no TLS checks or CI policies were weakened.API upload/<default>scan configuration. This is a scanning-coverage limitation, not a clean aggregate scan. The current-head automated review recommends approval with no open or suppressed findings; maintainer approval and contributor sign-off remain outstanding.Contributor validation, before the nested-metadata follow-up: hosted Docker run passed root style/compilation, both original regressions, all 116 VW tests without skips,
PipelineTestCoverageSuite, six-module codegen, core/VW Python package installation andpublishM2. Installed-Python checks covered scalar/array predictions, custom input, copy/save/load and collision handling. Its validation-branch SHA differs from the original PR head, but the tested candidate patch was independently verified to match that PR diff. It does not validate the maintainer follow-up.No Fabric or native Windows runtime validation is claimed.
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.npm startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.