#813 Fix compatibility of the relaxed sign overpunching. #814
Conversation
WalkthroughAdds a boolean flag Changes
Sequence Diagram(s)(omitted — changes are internal flag propagation and do not introduce multi-component sequential flows warranting a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scala (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (15)
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
🤖 Fix all issues with AI Agents
In
@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scala:
- Line 220: The call to StringDecoders.decodeEbcdicLong uses the old parameter
order; swap the last two boolean arguments so relaxedOverpunch precedes
improvedNullDetection — replace the current argument sequence "..., isSigned ||
!strictSignOverpunch, improvedNullDetection, !strictSignOverpunch" with "...,
isSigned || !strictSignOverpunch, !strictSignOverpunch, improvedNullDetection"
so the call matches the corrected decodeEbcdicLong signature.
In
@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala:
- Line 346: The method signature for decodeEbcdicLong has the last two boolean
parameters in the wrong order (improvedNullDetection, relaxedOvepunch) causing
callers to pass values incorrectly; change the parameter order to match the
other decoders (relaxedOvepunch, improvedNullDetection) in the decodeEbcdicLong
signature and update any references inside the method body that use those
parameter names so their semantics remain correct (use
decodeEbcdicInt/decodeAsciiLong as the correct ordering reference).
🧹 Nitpick comments (4)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (3)
22-22: Verify if this import is needed.The
StandardCharsetsimport was added but doesn't appear to be used in this file based on the changes shown.🔎 Verification script
#!/bin/bash # Check if StandardCharsets is actually used in this file rg -n 'StandardCharsets\.' cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala
165-165: Fix typo in documentation."multiple published signs" should be "multiple punched signs".
🔎 Proposed fix
- * @param relaxedOvepunch If true, multiple published signs are allowed. The last is going to be effective + * @param relaxedOvepunch If true, multiple punched signs are allowed. The last is going to be effective
165-165: Consider fixing typo in parameter name throughout the codebase.The parameter is consistently named
relaxedOvepunchbut should likely berelaxedOverpunch(missing 'r'). While this is consistent throughout the PR, it's still a typo that might cause confusion.This is a breaking API change, so it should be done carefully. If you decide to fix it, ensure all call sites are updated.
Also applies to: 238-238, 307-307, 324-324, 343-343, 360-360, 378-378, 398-398, 419-419, 438-438
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala (1)
171-171: Fix typo in test name."Test relaxed overpunchiung" should be "Test relaxed overpunching" (missing 'c').
🔎 Proposed fix
- "Test relaxed overpunchiung" when { + "Test relaxed overpunching" when {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala
🧰 Additional context used
🧬 Code graph analysis (4)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scala (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (9)
StringDecoders(25-449)decodeEbcdicBigNumber(384-390)decodeAsciiBigNumber(404-410)decodeEbcdicInt(310-316)decodeAsciiInt(328-334)decodeEbcdicLong(346-352)decodeAsciiLong(364-370)decodeEbcdicNumber(169-230)decodeAsciiNumber(242-298)cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/common/Constants.scala (1)
Constants(20-81)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala (1)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (1)
StringDecoders(25-449)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scala (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (10)
decodeEbcdicNumber(169-230)decodeAsciiNumber(242-298)decodeEbcdicInt(310-316)decodeAsciiInt(328-334)decodeEbcdicLong(346-352)decodeAsciiLong(364-370)decodeEbcdicBigNumber(384-390)decodeAsciiBigNumber(404-410)decodeEbcdicBigDecimal(423-429)decodeAsciiBigDecimal(442-448)cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/testutils/EbcdicEncoder.scala (1)
toEbcdic(37-46)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala (1)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/fixtures/BinaryFileFixture.scala (2)
withTempBinFile(60-69)withTempTextFile(42-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Spark 3.4.4 on Scala 2.12.20
- GitHub Check: Spark 2.4.8 on Scala 2.11.12
- GitHub Check: Spark 3.5.7 on Scala 2.13.17
- GitHub Check: Spark 3.5.7 on Scala 2.12.20
- GitHub Check: test (2.12.20, 2.12, 3.3.4, 0, 80, 20)
🔇 Additional comments (4)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scala (1)
131-138: LGTM: Correct mapping of strictSignOverpunch to relaxedOvepunch.The logic correctly maps
!strictSignOverpunchtorelaxedOvepunchacross all decoder calls. When strict sign overpunching is disabled, relaxed overpunching is enabled, which is the intended behavior.Also applies to: 210-227, 270-274
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala (1)
171-216: LGTM: Comprehensive test coverage for relaxed overpunching.The tests correctly verify relaxed overpunching behavior for both EBCDIC and ASCII:
- EBCDIC test: Bytes with multiple overpunched signs (
0xD0= -0,0xC4= +4) correctly decode to 1230.4, where the last sign (+) wins.- ASCII test: String "123}D " with multiple overpunched signs (
}= -0,D= +4) also correctly decodes to 1230.4.Both tests use the appropriate
strict_sign_overpunching=falsesetting and validate the expected behavior where multiple overpunched signs are allowed and the last one determines the final sign.cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scala (1)
633-916: LGTM: Consistent test updates for new parameter.All existing test calls have been correctly updated to include
relaxedOvepunch = false, maintaining the existing test behavior while adapting to the new API.cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala (1)
48-106: LGTM: Excellent test coverage for relaxed overpunching.The new tests thoroughly validate relaxed overpunching behavior:
EBCDIC tests (lines 48-58):
- Multiple overpunched signs in the middle of numbers
- Correct handling where the last sign wins
- Proper null return for unsigned negative numbers
ASCII tests (lines 101-106):
- Multiple overpunched characters anywhere in the string
- Correct sign determination based on the last overpunched character
- Proper validation of unsigned constraints
All test logic is sound and expectations are correct.
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scala
Outdated
Show resolved
Hide resolved
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In
@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala:
- Line 165: The ScalaDoc for the overpunch-related parameter(s) contains a typo:
replace the word "published" with the correct term "punched" wherever it appears
in StringDecoders.scala (e.g., the param description for relaxedOvepunch in the
StringDecoders object/class and the other listed comment occurrences). Update
the parameter descriptions (references like relaxedOvepunch and any Scaladoc
lines around it) to read "punched signs" (or "punched" as appropriate)
consistently for all occurrences (lines noted: 165, 238, 307, 324, 342, 360,
378, 398, 419, 438) so the documentation uses the established term "sign
overpunching."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala
🚧 Files skipped from review as they are similar to previous changes (2)
- spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test19SignOverpunching.scala
- cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/DecoderSelector.scala
🧰 Additional context used
🧬 Code graph analysis (1)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scala (1)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (10)
decodeEbcdicNumber(169-230)decodeAsciiNumber(242-298)decodeEbcdicInt(310-316)decodeAsciiInt(328-334)decodeEbcdicLong(346-352)decodeAsciiLong(364-370)decodeEbcdicBigNumber(384-390)decodeAsciiBigNumber(404-410)decodeEbcdicBigDecimal(423-429)decodeAsciiBigDecimal(442-448)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Spark 3.5.7 on Scala 2.12.20
- GitHub Check: Spark 3.4.4 on Scala 2.12.20
- GitHub Check: Spark 3.5.7 on Scala 2.13.17
- GitHub Check: Spark 2.4.8 on Scala 2.11.12
- GitHub Check: test (2.12.20, 2.12, 3.3.4, 0, 80, 20)
🔇 Additional comments (4)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala (3)
346-346: Confirmed: Parameter order issue resolved.The critical parameter order inconsistency flagged in the previous review has been fixed. The signature now correctly matches all other decoder methods with
relaxedOvepunchbeforeimprovedNullDetection.
169-230: LGTM: Relaxed overpunch logic for EBCDIC is sound.The implementation correctly allows multiple overpunched signs when
relaxedOvepunchis true by:
- Bypassing the strict post-sign validation (line 181)
- Accepting overpunched characters throughout the string (lines 197, 201)
The last encountered sign determines the final sign, as documented.
242-298: LGTM: ASCII relaxed overpunch logic is correct.The implementation properly distinguishes between relaxed and strict modes:
- Relaxed mode: Punched sign characters accepted anywhere in the string
- Strict mode: Punched signs restricted to first or last position only
Both modes return
nullfor unsupported characters, maintaining validation rigor.cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecodersSpec.scala (1)
633-916: LGTM: Test updates maintain backward compatibility.All decoder test calls have been systematically updated to include
relaxedOvepunch = false, preserving existing behavior and expectations. The mechanical consistency of these updates reduces the risk of introducing test regressions.
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/StringDecoders.scala
Outdated
Show resolved
Hide resolved
yruslan
force-pushed
the
bugfix/813-fix-relaxed-sign-overpunch-compatibility
branch
from
ce2d67e to
82080f9
Compare
yruslan
deleted the
bugfix/813-fix-relaxed-sign-overpunch-compatibility
branch
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.