#837 Fix "PIC SVPP9(5) COMP-3" when the scale factor is negative

[yruslan]

Closes #837

For

      10 N  PIC SVPP9(5) COMP-3.

bytes:

0x06, 0x54, 0x7C

Before

+---------+
|N        |
+---------+
|0.0000655|
+---------+

After

+---------+
|N        |
+---------+
|0.0006547|
+---------+

Summary by CodeRabbit

• Bug Fixes

  • Corrected decimal point and leading-zero placement for scaled decimals with negative scale factors in packed decimal (COMP-3) data.

• Tests

  • Added unit tests covering scaled COMP-3 decoding across different scales, negative scale factors, and sign nibble encodings.
  • Added parsing/regression tests validating copybook parsing, generated record layout, schema precision, and decoded decimal values.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f0e3c94c-5228-4520-bf8a-887e9e6dfe24

📥 Commits

Reviewing files that changed from the base of the PR and between cc9e480 and 940a157.

📒 Files selected for processing (2)

• cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala
• spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala

🚧 Files skipped from review as they are similar to previous changes (2)

• cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala
• spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala

---

Walkthrough

Adjusts COMP-3 decimal decoding to correct decimal point placement when scaleFactor is negative (e.g., PIC SVPP9(5)), and adds unit and regression tests covering scaled decimals and negative scaleFactor cases.

Changes

Cohort / File(s)	Summary
Decoder Logic cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/BCDNumberDecoders.scala	Introduces scaleDotPosition (uses scale - 1 when scaleFactor < 0 && scale > 0) and updates intendedDecimalPosition to use it, altering calculation of additionalZeros and decimalPointPosition for negative scaleFactor.
Parser Tests cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/copybooks/ParseCopybookFeaturesSpec.scala	Adds test for parsing scaled decimals (PIC SVPP9(5) COMP-3): asserts field is Decimal with scale == 5 and scaleFactor == -2, and verifies generated record layout.
Decoder Unit Tests cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala	Adds tests exercising BCDNumberDecoders.decodeBigBCDNumber across combinations of scale and negative scaleFactor, including sign nibble variants, verifying exact decoded decimal strings.
Spark Regression Test spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala	Adds regression test loading a binary copybook with PIC SVPP9(5) fields (COMP-3 and DISPLAY) and asserts Spark schema and decoded decimal values match expected results (0.0006547).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nibble nibbles, count each P,

scale slides left so decimals see,
COMP-3 bytes line up just so,
zeros find places, numbers glow,
tiny hops — precision grows 🌱

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically references the issue number and describes the exact fix for COMP-3 parsing with negative scale factors, matching the primary change in the changeset.
Linked Issues check	✅ Passed	The PR directly addresses issue #837 by fixing COMP-3 parsing for fields like 'PIC SVPP9(5)' with negative scale factors, implementing the core objective to correctly apply scale factors and produce expected decimal results.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing COMP-3 decimal parsing with negative scale factors: decoder logic, unit tests, integration tests, and regression tests all validate the fix without introducing unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/837-decimal-scale-factor-fix

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

JaCoCo code coverage report - 'cobol-parser'

Overall Project	91.44%	🍏
Files changed	100%	🍏

File	Coverage
BCDNumberDecoders.scala	79.78%	🍏

[github-actions]

JaCoCo code coverage report - 'spark-cobol'

Overall Project	83.32%	🍏

There is no coverage information present for the Files changed

[coderabbitai]

🧹 Nitpick comments (2)

cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala (1)

229-247: Strengthen regression assertions to exact matches.

Using contains can hide formatting regressions. For numeric decoder regressions, exact string equality is safer.

Suggested diff

-    assert (v1.contains("06547"))
+    assert(v1 == "06547")
...
-    assert (v2.contains("0.006547"))
+    assert(v2 == "0.006547")
...
-    assert (v3.contains("0.0006547"))
+    assert(v3 == "0.0006547")
...
-    assert (v4.contains("0.006547"))
+    assert(v4 == "0.006547")
...
-    assert (v5.contains("0.0006547"))
+    assert(v5 == "0.0006547")
...
-    assert (v6.contains("0.0016547"))
+    assert(v6 == "0.0016547")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala`
around lines 229 - 247, The test uses contains() which can mask formatting
regressions; update the assertions in the "Test COMP-3 decimal with scale factor
cases" test to assert exact string equality for each decoded value returned by
BCDNumberDecoders.decodeBigBCDNumber instead of contains; specifically assert
that v1 == "06547", v2 == "0.006547", v3 == "0.0006547", v4 == "0.006547", v5 ==
"0.0006547", and v6 == "0.0016547" (use your project's preferred equality
assertion helper such as assert(... == ...), assertResult, or shouldEqual).

spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala (1)

285-321: Great regression coverage; minor test cleanup suggested.

Nice end-to-end check for both COMP-3 and DISPLAY paths. You can avoid repeated collection by reading the first row once.

Suggested diff

-        val actualData1 = df.collect()(0).getDecimal(0).toString
-        val actualData2 = df.collect()(0).getDecimal(1).toString
+        val row = df.collect().head
+        val actualData1 = row.getDecimal(0).toString
+        val actualData2 = row.getDecimal(1).toString

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala`
around lines 285 - 321, The test repeatedly calls df.collect()(0) to extract
decimals into actualData1 and actualData2; change to collect the first row once
(e.g., val row = df.collect()(0) or df.first()) and then derive actualData1 =
row.getDecimal(0).toString and actualData2 = row.getDecimal(1).toString to avoid
duplicate collection and improve efficiency; update references to
df.collect()(0).getDecimal(...) accordingly (symbols: actualData1, actualData2,
df.collect()(0).getDecimal).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala`:
- Around line 229-247: The test uses contains() which can mask formatting
regressions; update the assertions in the "Test COMP-3 decimal with scale factor
cases" test to assert exact string equality for each decoded value returned by
BCDNumberDecoders.decodeBigBCDNumber instead of contains; specifically assert
that v1 == "06547", v2 == "0.006547", v3 == "0.0006547", v4 == "0.006547", v5 ==
"0.0006547", and v6 == "0.0016547" (use your project's preferred equality
assertion helper such as assert(... == ...), assertResult, or shouldEqual).

In
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala`:
- Around line 285-321: The test repeatedly calls df.collect()(0) to extract
decimals into actualData1 and actualData2; change to collect the first row once
(e.g., val row = df.collect()(0) or df.first()) and then derive actualData1 =
row.getDecimal(0).toString and actualData2 = row.getDecimal(1).toString to avoid
duplicate collection and improve efficiency; update references to
df.collect()(0).getDecimal(...) accordingly (symbols: actualData1, actualData2,
df.collect()(0).getDecimal).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce297ac8-256f-4e0e-9896-c3826e25df4f

📥 Commits

Reviewing files that changed from the base of the PR and between 4b9c055 and cc9e480.

📒 Files selected for processing (4)

• cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/decoders/BCDNumberDecoders.scala
• cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/copybooks/ParseCopybookFeaturesSpec.scala
• cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/decoders/BinaryDecoderSpec.scala
• spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test17NumericConversions.scala