[SPARK-58396][ML][CONNECT] Include FPGrowth metadata in size estimates - #57590
[SPARK-58396][ML][CONNECT] Include FPGrowth metadata in size estimates#57590zhengruifeng wants to merge 1 commit into
Conversation
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Thank you @zhengruifeng! Left just one comment, otherwise LGTM
| ParamsSuite.checkParams(model) | ||
| } | ||
|
|
||
| test("FPGrowthModel estimated size") { |
There was a problem hiding this comment.
The new test asserts only estimatedSize < 16 KiB (an upper bound). Because the training dataset is tiny (4 rows, 3 frequent items), this upper bound holds with or without the 6-line metadata addition; the test would pass if estimateMatadataSize were removed, so it does not guard the behavior it introduces. The same-day ALS sibling (SPARK-58367) used the stronger two-assertion pattern: first model.estimatedSize === estimatedDFSize + model.estimateMatadataSize to pin the metadata contribution, then the < 16 KiB bound. Recommend adding an equality assertion that binds estimateMatadataSize so the test is an actual regression guard. Note: the clustering siblings (SPARK-58361) used upper-bound-only and were merged without objection, so the author may consider this precedent sufficient, but the ALS test in the same series is the stronger template.
### What changes were proposed in this pull request? Adds parameter metadata to `FPGrowthModel.estimatedSize` while retaining its existing frequent-itemset and item-support estimates. Adds regression coverage using the suite's small FP-growth dataset. ### Why are the changes needed? The specialized estimate counted learned data but omitted the model's parameter metadata. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added `FPGrowthModel estimated size` coverage in `FPGrowthSuite`. The suite was not run locally. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Codex (GPT-5) Closes #57590 from zhengruifeng/fpgrowth-model-size-metadata-dev3. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org> (cherry picked from commit 233f1a4) Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>
2 participants
What changes were proposed in this pull request?
Adds parameter metadata to
FPGrowthModel.estimatedSizewhile retaining its existing frequent-itemset and item-support estimates. Adds regression coverage using the suite's small FP-growth dataset.Why are the changes needed?
The specialized estimate counted learned data but omitted the model's parameter metadata.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added
FPGrowthModel estimated sizecoverage inFPGrowthSuite. The suite was not run locally.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Codex (GPT-5)