Skip to content

Conversation

andy-hf-kwok
Copy link

Which issue does this PR close?

Partially closes #.#2255

Rationale for this change

What changes are included in this PR?

  • Add explicit cast (int -> long) to avoid the warning
  • Add a suppress warning on shim and put up a comment to justify it.

How are these changes tested?

  • mvn test -pl :comet-common-spark3.5_2.12 -Pstrict-warnings to make sure the common module is now comply with the strict-warnings profile setting
  • CI is green

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2025

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.00%. Comparing base (f09f8af) to head (e2a67ae).
⚠️ Report is 579 commits behind head on main.

Files with missing lines Patch % Lines
.../scala/org/apache/spark/sql/comet/util/Utils.scala 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2527      +/-   ##
============================================
+ Coverage     56.12%   59.00%   +2.88%     
- Complexity      976     1457     +481     
============================================
  Files           119      146      +27     
  Lines         11743    13566    +1823     
  Branches       2251     2358     +107     
============================================
+ Hits           6591     8005    +1414     
- Misses         4012     4340     +328     
- Partials       1140     1221      +81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


if (out.size() > 0) {
(batch.numRows(), cbbos.toChunkedByteBuffer)
(batch.numRows().toLong, cbbos.toChunkedByteBuffer)
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the warning for this change?

Copy link
Author

Choose a reason for hiding this comment

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

/scala/org/apache/spark/sql/comet/util/Utils.scala:226: implicit numeric widening

The type mismatch of batch.numRows() returning int, compare to the method return type Iterator[(Long, ChunkedByteBuffer)], which is long.
By working on this EPIC, I realized there are some more widening issue on the spark module also, sure we can probably suppress the warning, since this is a up cast, but if adding .toLong or equivalent would dismiss the warning all together I would reckon it would be better.
Being explicit on the intension is always better.

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.

3 participants