[SPARK-58251][ML][CONNECT] Add size estimates for OneHotEncoderModel, PCAModel, StringIndexerModel, and IDFModel#57422
Conversation
zhengruifeng
changed the title
zhengruifeng
changed the title
| ParamsSuite.checkParams(model) | ||
| } | ||
|
|
||
| test("IDFModel estimated size") { |
There was a problem hiding this comment.
Each new test asserts only an upper bound (estimatedSize < 2KB/4KB). This exercises the code path (guards against NPE / session traversal) but a regression that dropped a learned-state component or returned 0 would still pass all four. A > 0 lower bound, or a "grows with data" check across two input sizes (as in ModelSuite's Word2Vec test and the relative-accuracy checks in LogisticRegressionSuite), would give real regression protection for these four new overrides. Low risk given the implementations are trivial, but it is the one actionable gap for a test-carrying change.
There was a problem hiding this comment.
Thanks. These tests are deliberately focused on ensuring that estimatedSize does not unexpectedly retain or traverse a reference to a large object; that is the regression they are intended to catch. For PCAModel, pc is never null when the model is fitted or loaded and can only be null through the private[ml] constructor, so the null check is only a defensive safeguard.
… PCAModel, StringIndexerModel, and IDFModel ### What changes were proposed in this pull request? Add explicit `estimatedSize` implementations for OneHotEncoderModel, PCAModel, StringIndexerModel, and IDFModel. Each directly accounts for model metadata and model-owned learned state; vectors and matrices use `getSizeInBytes`. ### Why are the changes needed? SPARK-57521 already makes the default `Model.estimatedSize` estimate a parentless copy, avoiding parent-estimator and SparkSession traversal. These explicit implementations make size accounting cheaper and more precise for feature models, and align them with models that already provide specialized estimates. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Added fitted-model size bounds to the corresponding feature algorithm suites. - `JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 build/sbt 'mllib/Test/compile'`\n- Targeted suites were not run.\n\n### Was this patch authored or co-authored using generative AI tooling?\n\nGenerated-by: OpenAI Codex (GPT-5) Closes #57422 from zhengruifeng/feature-model-size-estimates-master-dev3. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org> (cherry picked from commit 36d311f) Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>
3 participants
What changes were proposed in this pull request?
Add explicit
estimatedSizeimplementations for OneHotEncoderModel, PCAModel, StringIndexerModel, and IDFModel. Each directly accounts for model metadata and model-owned learned state; vectors and matrices usegetSizeInBytes.Why are the changes needed?
SPARK-57521 already makes the default
Model.estimatedSizeestimate a parentless copy, avoiding parent-estimator and SparkSession traversal. These explicit implementations make size accounting cheaper and more precise for feature models, and align them with models that already provide specialized estimates.Does this PR introduce any user-facing change?
No.
How was this patch tested?
JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 build/sbt 'mllib/Test/compile'\n- Targeted suites were not run.\n\n### Was this patch authored or co-authored using generative AI tooling?\n\nGenerated-by: OpenAI Codex (GPT-5)