Skip to content

fix: use 64-bit bulk sparse row offsets - #2790

Merged
Rana Singh (ranadeepsingh) merged 2 commits into
microsoft:masterfrom
leninworld:fix-bulk-sparse-row-offsets-issue-2786
Oct 28, 2026
Merged

Rana Singh (ranadeepsingh) merged 2 commits into
microsoft:masterfrom
leninworld:fix-bulk-sparse-row-offsets-issue-2786

Conversation

@leninworld

@leninworld Lenin Mookiah (leninworld) commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

This PR fixes issue #2786.

What changes are proposed in this pull request?

Bulk sparse aggregation currently stores cumulative CSR row offsets as 32-bit integers. A total nonzero count above Int.MaxValue can wrap before the native dataset is created. Store cumulative offsets in a LongSwigArray, widen the chunked row counts before accumulation in both dataset modes, and pass them to LightGBM as C_API_DTYPE_INT64. Feature indexes and per-row counts retain their existing types. Also corrects the sparse-dataset log message (previously said “dense”), makes the prefix helper package-private for tests, and doubles index-pointer storage per row from 4 to 8 bytes.

Add 13 regressions for prefix boundaries (including totals beyond Int.MaxValue), empty and chunked inputs, both aggregation modes, and native bulk/streaming prediction and save/load parity.

How is this patch tested?

  • I have written tests and confirmed the proposed fix works.

Validation on Linux x86-64 (emulated), Java 11.0.32, Scala 2.12.17 and Spark 3.5.0:

  • Core and LightGBM main/test compilation and all four Scala style checks passed using retained incremental build caches.
  • All 13 targeted regressions passed, with no ignored tests.
  • Core/LightGBM Python wrapper generation and JAR packaging passed. Generated Python wrappers loaded with the branch-built JARs and trained sparse bulk models in both dataset modes; 64 predictions per mode matched the saved/reloaded model within 1e-9.
  • Network-isolated validation reused cached dependencies plus local POM metadata; cache audit found no new/changed JARs or successful remote downloads.

The original compiled prefix overflow was reproduced separately. The reported Databricks crash, the full repository suite and Databricks runtime matrix were not reproduced/run. Earlier validation setup failures were corrected in the harness; they are not product changes.

Validation summary

Does this PR change any dependencies?

  • No.

Does this PR add a new feature? If so, have you added samples on website?

  • No. This corrects existing sparse bulk aggregation.

Related Issues/PRs

Fixes #2786

Copilot AI balanced review requested due to automatic review settings October 25, 2026 14:56
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions

Copy link
Copy Markdown

Hey Lenin Mookiah (@leninworld) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.

We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!

Examples of commit messages with semantic prefixes:

  • fix: Fix LightGBM crashes with empty partitions
  • feat: Make HTTP on Spark back-offs configurable
  • docs: Update Spark Serving usage
  • build: Add codecov support
  • perf: improve LightGBM memory usage
  • refactor: make python code generation rely on classes
  • style: Remove nulls from CNTKModel
  • test: Add test coverage for CNTKModel

To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new aggregation regression introduces an RDD dependency contrary to the repository’s mandatory DataFrame/Dataset-only requirement.

1 open issue
What changed in this PR

Fixes #2786 by using 64-bit cumulative row offsets in LightGBM’s bulk sparse aggregation, preventing overflow when stored-value totals exceed Int.MaxValue.

Changes:

  • Widens row counts before accumulation in both dataset modes and passes offsets to native LightGBM as C_API_DTYPE_INT64.
  • Corrects the sparse-dataset log message.
  • Adds 13 regressions covering offset boundaries, chunk handling, aggregation modes, and bulk/streaming prediction and persistence parity.
File Description
lightgbm/​src/​test/​scala/​com/​microsoft/​azure/​synapse/​ml/​lightgbm/​split1/​BulkSparseOffsetsSuite.scala Adds boundary, aggregation, native training, and persistence regressions.
lightgbm/​src/​main/​scala/​com/​microsoft/​azure/​synapse/​ml/​lightgbm/​dataset/​DatasetAggregator.scala Uses 64-bit sparse offsets throughout aggregation and native dataset creation.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 25, 2026 15:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Offset storage, accumulation, and native type arguments are consistent across both aggregation modes, with focused regression coverage and no unresolved findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@ranadeepsingh

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@ranadeepsingh
Rana Singh (ranadeepsingh) merged commit e58441f into microsoft:master Oct 28, 2026
76 checks passed
@ranadeepsingh

Copy link
Copy Markdown
Collaborator

Thanks Lenin Mookiah (@leninworld) for the fix and the care you put into the regression tests. The overflow boundary cases and the checks against streaming predictions made this much easier to review. This addresses a nasty executor crash for people training on large sparse datasets. I appreciate you following through on the review feedback, too. Glad to have this merged!

@leninworld

Copy link
Copy Markdown
Contributor Author

Thanks Lenin Mookiah (@leninworld) for the fix and the care you put into the regression tests. The overflow boundary cases and the checks against streaming predictions made this much easier to review. This addresses a nasty executor crash for people training on large sparse datasets. I appreciate you following through on the review feedback, too. Glad to have this merged!

Glad to contribute and Thanks Rana Singh (@ranadeepsingh)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] LightGBM bulk mode passes int32 CSR row offsets, so a rank with more than 2^31-1 stored values crashes with SIGSEGV

4 participants