#853 Fix IBM single precision floating point (COMP-1) decoder to produce correct results.#854
Conversation
…uce correct results.
Walkthrough
ChangesIBM HFP Single-Precision Decoder Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JaCoCo code coverage report - 'cobol-parser'
|
JaCoCo code coverage report - 'spark-cobol'
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scala`:
- Around line 127-140: The byte extraction and zero-check logic (extracting b0,
b1, b2, b3 from the bytes array and checking if their combined value is zero) is
occurring before the try block, which means null or short input arrays can cause
unhandled exceptions that bypass the NonFatal exception guard. Move the byte
extraction statements and the zero-check condition into the try block so that
any potential IndexOutOfBoundsException or NullPointerException is properly
caught and handled according to the decoder's null-on-failure contract.
In
`@cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scala`:
- Around line 128-131: The test for IBM single-precision little-endian infinity
is calling the wrong decoder function. In the assertion line with
decodeIbmSingleBigEndian, replace this call with decodeIbmSingleLittleEndian to
correctly test the little-endian decoder. Ensure the byte array in the test is
adjusted to represent the proper little-endian byte order for infinity so the
test exercises the intended decoder and validates the correct behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1134243f-ee11-4b06-b9ec-3e6dcdb20f15
📒 Files selected for processing (3)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test03IbmFloats.scala
…d by microbenchmarking).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scala`:
- Around line 156-157: The overflow handling in the condition `convertedExp >
254` unconditionally returns `POSITIVE_INFINITY`, discarding the sign bit that
was extracted earlier at line 136. Modify this section to preserve the sign
during overflow by checking the sign bit and returning either
`POSITIVE_INFINITY` or `NEGATIVE_INFINITY` accordingly, so that negative values
that overflow produce `NEGATIVE_INFINITY` instead of `POSITIVE_INFINITY`,
consistent with IEEE 754 behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3e74525-fca4-4f7c-a91f-2966eead09ef
📒 Files selected for processing (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scala
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scala`:
- Around line 156-157: The overflow handling in the condition `convertedExp >
254` unconditionally returns `POSITIVE_INFINITY`, discarding the sign bit that
was extracted earlier at line 136. Modify this section to preserve the sign
during overflow by checking the sign bit and returning either
`POSITIVE_INFINITY` or `NEGATIVE_INFINITY` accordingly, so that negative values
that overflow produce `NEGATIVE_INFINITY` instead of `POSITIVE_INFINITY`,
consistent with IEEE 754 behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3e74525-fca4-4f7c-a91f-2966eead09ef
📒 Files selected for processing (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecodersSpec.scala
🛑 Comments failed to post (1)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scala (1)
156-157:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🌐 Web query:
ibm2ieee library overflow handling sign bit💡 Result:
The ibm2ieee library handles overflow and sign bit information in accordance with IEEE 754 standards for floating-point conversion [1][2]. Regarding overflow, the library is designed to identify when a rounded converted value exceeds the representable range of the target IEEE 754 type [1][2]. In such cases, the library returns an appropriately-signed infinity, ensuring that the sign bit is preserved despite the overflow [1][2]. This handling applies to both single-precision and double-precision conversions, though the risk of overflow depends on the specific format conversion (e.g., IBM single-precision has a larger range than IEEE 754 single-precision, while IBM double-precision has a smaller range than IEEE 754 double-precision) [1][2]. Regarding the sign bit, the library explicitly preserves the sign of the value throughout the conversion process [1][2]. This includes the handling of signed zeros, which are representable in both IBM and IEEE formats; the library ensures the sign bit of a zero is correctly maintained under all conversions [1][2].
Citations:
Sign bit lost on overflow — negative overflows become
POSITIVE_INFINITY.When
convertedExp > 254, the code unconditionally returnsPOSITIVE_INFINITY, discarding the sign extracted at line 136. A very large negative IBM HFP value will incorrectly decode as positive infinity instead of negative infinity. Per IEEE 754 and the ibm2ieee reference implementation, the sign bit must be preserved during overflow to return appropriately-signed infinity.Proposed fix
} else if (convertedExp > 254) { - java.lang.Float.POSITIVE_INFINITY + if (sign != 0) java.lang.Float.NEGATIVE_INFINITY else java.lang.Float.POSITIVE_INFINITY📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} else if (convertedExp > 254) { if (sign != 0) java.lang.Float.NEGATIVE_INFINITY else java.lang.Float.POSITIVE_INFINITY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/FloatingPointDecoders.scala` around lines 156 - 157, The overflow handling in the condition `convertedExp > 254` unconditionally returns `POSITIVE_INFINITY`, discarding the sign bit that was extracted earlier at line 136. Modify this section to preserve the sign during overflow by checking the sign bit and returning either `POSITIVE_INFINITY` or `NEGATIVE_INFINITY` accordingly, so that negative values that overflow produce `NEGATIVE_INFINITY` instead of `POSITIVE_INFINITY`, consistent with IEEE 754 behavior.
1 participant
Closes #853
- The original mask was incorrect (same as sign mask)
- Proper mask now extracts the 7-bit IBM exponent correctly
- IBM uses base-16 exponent bias of 64 vs IEEE-754's base-2 bias of 127
- Correct formula: ieee_exp = (ibm_exp - 64) * 4 + 127 - leading_zeros
- Simplified to: ibm_exp * 4 - 131 - leading_zeros
Test Updates