fix: make EnsembleByKey transformSchema match output - #2575
fix: make EnsembleByKey transformSchema match output#2575Minh Vu (fallintoplace) wants to merge 6 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Minh Vu (@fallintoplace) 👋! 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.
Pull request overview
This PR updates EnsembleByKey’s transformSchema logic so it matches the actual output schema produced by transform, across both collapseGroup=true and collapseGroup=false modes, and adds a regression test to validate schema consistency.
Changes:
- Rebuild
transformSchemaoutput fields usingkeysfor grouping fields and consistent output ordering for both collapse modes. - In non-collapsing mode, model Spark join column ordering and drop/replace conflicting output column names in the predicted schema.
- Add a test asserting
transformSchema(schema)equalstransform(df).schemafor both collapse modes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/main/scala/com/microsoft/azure/synapse/ml/stages/EnsembleByKey.scala | Adjusts transformSchema field selection and ordering to align with transform behavior. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/stages/EnsembleByKeySuite.scala | Adds schema-equality coverage for both collapseGroup modes. |
32aee3b to
0f15e8c
Compare
0f15e8c to
bdc39f9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2575 +/- ##
==========================================
+ Coverage 86.33% 86.36% +0.02%
==========================================
Files 335 335
Lines 18007 18250 +243
Branches 1658 1696 +38
==========================================
+ Hits 15546 15761 +215
- Misses 2461 2489 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI follow-up for |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
AB#2575 ## Summary Add Round 3 code review artifact for SynapseML PR microsoft#2575 focusing on edge cases and robustness. ## Prompting Intent Perform Round 3 of the DIRECT sequential six-round review gauntlet for SynapseML PR microsoft#2575. Focus on edge cases, robustness, error handling, boundary conditions, concurrency/session behavior, and failure modes. ## Linked Sources - Prompt: C:\Users\singhrana\.copilot\session-state\c2ea157b-c36a-4cbc-896d-e8caa3ea05bd\files\pr-2575\prompts\review-round-3.md - Review artifact: C:\Users\singhrana\Documents\SynapseML-pr-2575\reviews\pr-2575\task-2575-attempt-1-review-3-gemini-3.6-flash.md ## Rationale Recorded findings for edge cases, null join behavior, session case sensitivity, and vector aggregate nullability in the Round 3 review artifact as instructed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Make EnsembleByKey schema resolution match Spark output for collapsed and non-collapsed transforms, including scalar and vector means, nested and qualified keys, duplicate attributes, null keys, overwrite behavior, output naming, metadata, nullability, and unsupported inputs. Add generated-Python compatibility coverage and preserve the complete six-round review audit trail. ## Prompting Intent Take end-to-end ownership of GitHub PR microsoft#2575 in an isolated worktree. Independently verify and harden EnsembleByKey schema behavior, run the required six-round DIRECT sequential multi-model review, fix and rerun every finding until clean, validate Scala and generated-language surfaces, avoid generated target edits and history rewrites, and prepare the existing PR branch for normal push and Azure validation. ## Linked Sources - Pull request: microsoft#2575 - Round 1 review: reviews/pr-2575/task-2575-attempt-1-review-1-claude-opus-5.md - Round 2 review: reviews/pr-2575/task-2575-attempt-1-review-2-gpt-5.6-sol.md - Round 3 review: reviews/pr-2575/task-2575-attempt-1-review-3-gemini-3.6-flash.md - Round 4 review: reviews/pr-2575/task-2575-attempt-1-review-4-claude-opus-5.md - Round 5 review: reviews/pr-2575/task-2575-attempt-1-review-5-gpt-5.6-sol.md - Round 6 review: reviews/pr-2575/task-2575-attempt-1-review-6-gemini-3.6-flash.md ## Rationale Keep business logic Scala-first and derive the Python surface through SynapseML's internal-wrapper override pattern. Resolve configured fields against schema ordinals and analyzed Spark attributes so transformSchema remains optimistic where qualifier or ExprId identity is unavailable while transform rejects genuine runtime ambiguity. Use positional normalization and public DataFrame APIs for Spark 3.5/4.1 compatibility, and use null-safe joins to preserve non-collapsed rows with null grouping keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Remove the six generated review audit files from the committed PR contents while leaving all reviewed implementation, documentation, and regression tests unchanged. ## Prompting Intent The engineer asked to prevent the generated review files from being merged. Preserve the existing PR history, avoid force-pushing, and remove only those committed artifacts. ## Linked Sources - Pull request: microsoft#2575 - User request: remove the review files from the committed files ## Rationale Use a follow-up deletion commit rather than rewriting the existing commit so the shared PR branch remains safe and auditable. The review process remains reflected in the conversation and prior commit history, but the final merged tree contains only the product changes and their tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Merge upstream master at `617ad0f8fc8932d93fc6185cb2547e5ab2c2719a` into the existing EnsembleByKey PR branch. ## Prompting Intent The engineer asked to update the published PR branch with the latest master changes. Preserve branch history and use a normal push rather than rebasing or force-pushing. ## Linked Sources - Pull request: microsoft#2575 - Upstream repository: https://github.com/microsoft/SynapseML ## Rationale Use a merge commit because the branch is already published and the engineer selected the history-preserving option. This incorporates current upstream changes without rewriting commits reviewed and validated on the shared PR branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
core/src/test/scala/com/microsoft/azure/synapse/ml/stages/EnsembleByKeySuite.scala:745
withSQLConfalways restores viaspark.conf.set(configName, previousValue), which can (a) throw if the key was unset/nonexistent and (b) permanently convert an unset config into a user override. Other tests in this repo preserve the unset/set state viagetOption+unset(e.g., core/src/test/scala/com/microsoft/azure/synapse/ml/recommendation/SARIdentifierSpec.scala:152-181).
private def withSQLConf[T](configName: String, value: String)(action: => T): T = {
val previousValue = spark.conf.get(configName)
spark.conf.set(configName, value)
try action finally spark.conf.set(configName, previousValue)
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
AB#2575 ## Summary Add Round 3 code review artifact for SynapseML PR microsoft#2575 focusing on edge cases and robustness. ## Prompting Intent Perform Round 3 of the DIRECT sequential six-round review gauntlet for SynapseML PR microsoft#2575. Focus on edge cases, robustness, error handling, boundary conditions, concurrency/session behavior, and failure modes. ## Linked Sources - Prompt: C:\Users\singhrana\.copilot\session-state\c2ea157b-c36a-4cbc-896d-e8caa3ea05bd\files\pr-2575\prompts\review-round-3.md - Review artifact: C:\Users\singhrana\Documents\SynapseML-pr-2575\reviews\pr-2575\task-2575-attempt-1-review-3-gemini-3.6-flash.md ## Rationale Recorded findings for edge cases, null join behavior, session case sensitivity, and vector aggregate nullability in the Round 3 review artifact as instructed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Make EnsembleByKey schema resolution match Spark output for collapsed and non-collapsed transforms, including scalar and vector means, nested and qualified keys, duplicate attributes, null keys, overwrite behavior, output naming, metadata, nullability, and unsupported inputs. Add generated-Python compatibility coverage and preserve the complete six-round review audit trail. ## Prompting Intent Take end-to-end ownership of GitHub PR microsoft#2575 in an isolated worktree. Independently verify and harden EnsembleByKey schema behavior, run the required six-round DIRECT sequential multi-model review, fix and rerun every finding until clean, validate Scala and generated-language surfaces, avoid generated target edits and history rewrites, and prepare the existing PR branch for normal push and Azure validation. ## Linked Sources - Pull request: microsoft#2575 - Round 1 review: reviews/pr-2575/task-2575-attempt-1-review-1-claude-opus-5.md - Round 2 review: reviews/pr-2575/task-2575-attempt-1-review-2-gpt-5.6-sol.md - Round 3 review: reviews/pr-2575/task-2575-attempt-1-review-3-gemini-3.6-flash.md - Round 4 review: reviews/pr-2575/task-2575-attempt-1-review-4-claude-opus-5.md - Round 5 review: reviews/pr-2575/task-2575-attempt-1-review-5-gpt-5.6-sol.md - Round 6 review: reviews/pr-2575/task-2575-attempt-1-review-6-gemini-3.6-flash.md ## Rationale Keep business logic Scala-first and derive the Python surface through SynapseML's internal-wrapper override pattern. Resolve configured fields against schema ordinals and analyzed Spark attributes so transformSchema remains optimistic where qualifier or ExprId identity is unavailable while transform rejects genuine runtime ambiguity. Use positional normalization and public DataFrame APIs for Spark 3.5/4.1 compatibility, and use null-safe joins to preserve non-collapsed rows with null grouping keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Remove the six generated review audit files from the committed PR contents while leaving all reviewed implementation, documentation, and regression tests unchanged. ## Prompting Intent The engineer asked to prevent the generated review files from being merged. Preserve the existing PR history, avoid force-pushing, and remove only those committed artifacts. ## Linked Sources - Pull request: microsoft#2575 - User request: remove the review files from the committed files ## Rationale Use a follow-up deletion commit rather than rewriting the existing commit so the shared PR branch remains safe and auditable. The review process remains reflected in the conversation and prior commit history, but the final merged tree contains only the product changes and their tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a8d5521 to
04d48a1
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AB#2575 ## Summary Add Round 3 code review artifact for SynapseML PR microsoft#2575 focusing on edge cases and robustness. ## Prompting Intent Perform Round 3 of the DIRECT sequential six-round review gauntlet for SynapseML PR microsoft#2575. Focus on edge cases, robustness, error handling, boundary conditions, concurrency/session behavior, and failure modes. ## Linked Sources - Prompt: C:\Users\singhrana\.copilot\session-state\c2ea157b-c36a-4cbc-896d-e8caa3ea05bd\files\pr-2575\prompts\review-round-3.md - Review artifact: C:\Users\singhrana\Documents\SynapseML-pr-2575\reviews\pr-2575\task-2575-attempt-1-review-3-gemini-3.6-flash.md ## Rationale Recorded findings for edge cases, null join behavior, session case sensitivity, and vector aggregate nullability in the Round 3 review artifact as instructed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Make EnsembleByKey schema resolution match Spark output for collapsed and non-collapsed transforms, including scalar and vector means, nested and qualified keys, duplicate attributes, null keys, overwrite behavior, output naming, metadata, nullability, and unsupported inputs. Add generated-Python compatibility coverage and preserve the complete six-round review audit trail. ## Prompting Intent Take end-to-end ownership of GitHub PR microsoft#2575 in an isolated worktree. Independently verify and harden EnsembleByKey schema behavior, run the required six-round DIRECT sequential multi-model review, fix and rerun every finding until clean, validate Scala and generated-language surfaces, avoid generated target edits and history rewrites, and prepare the existing PR branch for normal push and Azure validation. ## Linked Sources - Pull request: microsoft#2575 - Round 1 review: reviews/pr-2575/task-2575-attempt-1-review-1-claude-opus-5.md - Round 2 review: reviews/pr-2575/task-2575-attempt-1-review-2-gpt-5.6-sol.md - Round 3 review: reviews/pr-2575/task-2575-attempt-1-review-3-gemini-3.6-flash.md - Round 4 review: reviews/pr-2575/task-2575-attempt-1-review-4-claude-opus-5.md - Round 5 review: reviews/pr-2575/task-2575-attempt-1-review-5-gpt-5.6-sol.md - Round 6 review: reviews/pr-2575/task-2575-attempt-1-review-6-gemini-3.6-flash.md ## Rationale Keep business logic Scala-first and derive the Python surface through SynapseML's internal-wrapper override pattern. Resolve configured fields against schema ordinals and analyzed Spark attributes so transformSchema remains optimistic where qualifier or ExprId identity is unavailable while transform rejects genuine runtime ambiguity. Use positional normalization and public DataFrame APIs for Spark 3.5/4.1 compatibility, and use null-safe joins to preserve non-collapsed rows with null grouping keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Remove the six generated review audit files from the committed PR contents while leaving all reviewed implementation, documentation, and regression tests unchanged. ## Prompting Intent The engineer asked to prevent the generated review files from being merged. Preserve the existing PR history, avoid force-pushing, and remove only those committed artifacts. ## Linked Sources - Pull request: microsoft#2575 - User request: remove the review files from the committed files ## Rationale Use a follow-up deletion commit rather than rewriting the existing commit so the shared PR branch remains safe and auditable. The review process remains reflected in the conversation and prior commit history, but the final merged tree contains only the product changes and their tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
04d48a1 to
68f56a1
Compare
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
core/src/main/scala/com/microsoft/azure/synapse/ml/stages/EnsembleByKey.scala:276
- The comment claims RowOrdering.isOrderable is identical in Spark 3.5 and Spark 4.1, but SynapseML is pinned to Spark 3.5.0 (see build.sbt). Referencing Spark 4.1 here is likely to become misleading over time; it’s clearer to describe this as depending on Spark’s internal catalyst API and needing to stay aligned with the supported Spark version.
// Spark's GetMapValue casts the requested literal to the map key type and additionally requires
// that key type to be orderable (TypeUtils.checkForOrderingExpr -> RowOrdering.isOrderable).
// RowOrdering.isOrderable(DataType) is identical in Spark 3.5 and Spark 4.1, so it is safe here.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Problem
EnsembleByKey.transformSchemadescribes the opposite shape fromtransform:collapseGroup=true,transformreturns grouping keys and aggregate columns, whiletransformSchemakeeps every input columncollapseGroup=false,transformjoins aggregates back to the input, whiletransformSchemakeeps only fields selected from the aggregation column setThis can break pipeline schema validation because the declared schema differs from the DataFrame produced at runtime. The old implementation also fails when
transformSchemais called beforetransformwithout explicitcolNames, and declaresFloatTypefor scalar means that Spark produces asDoubleType.Changes
transformandtransformSchemakeysDoubleTypefor both float and double inputsTesting
core/testOnly com.microsoft.azure.synapse.ml.stages.EnsembleByKeySuite(9 tests passed)core/Test/scalastyle(0 findings)core/Compile/scalastyle(0 findings)