#801 Allow gaps between segment redefined fields as long as they have a single common base redefine. #802
Conversation
… a single common base redefine.
WalkthroughThis PR modifies the COBOL parser's segment redefine validation logic to permit non-contiguous segment redefines (allowing gaps) while maintaining validation that all gap-separated segments belong to the same redefine group and satisfy grouping constraints. Changes
Sequence DiagramsequenceDiagram
participant Parser as Segment Redefine Processor
participant GroupField as Group Field (Nested)
participant Validator as Validation Logic
rect rgb(220, 240, 255)
Note over Parser,Validator: Processing Group Within Redefine State (redefineGroupState == 1)
end
alt Group is itself a Redefine (g.redefines nonEmpty)
Parser->>Validator: Check if field is valid redefine
alt Field is REDEFINE or redefined by another
Validator-->>Parser: ✓ Valid, continue
else Field not in redefine group
Validator-->>Parser: ✗ Throw IllegalStateException
end
else Group is not a Redefine
Parser->>Validator: Check if field might be a redefine<br/>(fieldMightBeRedefine = true)
Validator-->>Parser: Pass flag to ensure validation
Note over Parser: "Allow redefines in between<br/>segment redefines"
alt redefineGroupState == 0 (outside active redefine)
Parser->>GroupField: Recurse into processGroupFields
else redefineGroupState != 0 (inside active redefine)
Parser->>GroupField: Return field as-is<br/>(no deeper processing)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/parse/SegmentRedefinesSpec.scala (1)
91-116: Test coverage for gaps feature is good but could be more thorough.The test correctly validates that SEGMENT-A, SEGMENT-C, and SEGMENT-D are marked as segment redefines while SEGMENT-B (the gap) is not. However, the assertions only check 4 of the 6 children in the copybook.
Consider adding assertions for completeness:
assert(!parsedCopybook.ast.children.head.asInstanceOf[Group].children(0).asInstanceOf[Group].isSegmentRedefine) assert(parsedCopybook.ast.children.head.asInstanceOf[Group].children(1).asInstanceOf[Group].isSegmentRedefine) assert(!parsedCopybook.ast.children.head.asInstanceOf[Group].children(2).asInstanceOf[Group].isSegmentRedefine) assert(parsedCopybook.ast.children.head.asInstanceOf[Group].children(3).asInstanceOf[Group].isSegmentRedefine) + assert(parsedCopybook.ast.children.head.asInstanceOf[Group].children(4).asInstanceOf[Group].isSegmentRedefine) + assert(!parsedCopybook.ast.children.head.asInstanceOf[Group].children(5).asInstanceOf[Group].isSegmentRedefine)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/asttransform/SegmentRedefinesMarker.scala(1 hunks)cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/parse/SegmentRedefinesSpec.scala(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/asttransform/SegmentRedefinesMarker.scala (1)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/ast/Group.scala (1)
withUpdatedIsSegmentRedefine(102-104)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/parse/SegmentRedefinesSpec.scala (2)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/CopybookParser.scala (2)
CopybookParser(42-490)parseTree(198-241)cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/policies/FillerNamingPolicy.scala (2)
FillerNamingPolicy(21-32)SequenceNumbers(22-22)
⏰ 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.5.7 on Scala 2.13.17
- GitHub Check: Spark 3.4.4 on Scala 2.12.20
- 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 (5)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/asttransform/SegmentRedefinesMarker.scala (3)
76-77: Validation logic correctly enforces single redefine group.The check ensures that once inside a redefine group (state=1), any new segment redefine field must itself be a REDEFINE (have a non-empty
redefinesclause) or be redefined by another field. This prevents mixing different redefine groups and correctly enforces that gaps are only allowed within the same redefine family.The condition
g.redefines.isEmptyis checked whenredefineGroupState == 1, meaning after the base segment redefine has been encountered. Since the base field (first in the group) doesn't have a redefines clause, it sets state to 1. Any subsequent segment redefine without a redefines clause would indicate a new redefine group, which is correctly rejected here.
83-88: Gap-allowing logic correctly implemented.The
fieldMightBeRedefineflag enables the key feature: when inside a redefine group (state=1), fields that are themselves redefines (have non-emptyredefinesclause) are treated as potential segment redefines. This prevents the state from transitioning to 2, allowing subsequent segment redefines to be found even when there are intervening redefine fields.The logic correctly distinguishes between:
- Fields that are redefines (allowed as gaps):
fieldMightBeRedefine = true- Fields that are not redefines (would end the group):
fieldMightBeRedefine = false
90-94: Recursion guard aligns with segment redefine level constraints.The change to only recurse when
redefineGroupState == 0is correct. Once segment redefines are found at the current level (state > 0), there's no need to search deeper levels since the validation rules require "all segment fields should belong to the level 1 (one level down record root level)". This optimization prevents unnecessary recursion while maintaining correct validation.cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/parse/SegmentRedefinesSpec.scala (2)
145-168: Test correctly validates that base segment must be redefined.This test ensures that SEGMENT-A (which is not redefined by any field and doesn't redefine another field) cannot be used as a segment redefine. The expected error message correctly identifies that SEGMENT_A was not found in the valid redefine structure.
The copybook structure creates two separate redefine groups:
- SEGMENT-B (base) → SEGMENT-C → SEGMENT-D
- SEGMENT-A (standalone, not part of any redefine group)
Since segment redefines must all belong to the same redefine group, SEGMENT-A is correctly rejected.
170-193: Test correctly validates no non-redefined fields between segment redefines.This test ensures that SEGMENT-C (which is a base field that doesn't redefine anything, even though it's redefined by SEGMENT-D) cannot appear between segment redefines from a different group. The error message correctly identifies that SEGMENT_C is not a REDEFINE or redefined by another field in the same group as SEGMENT-A and SEGMENT-B.
The validation enforces that once inside a redefine group, only fields that are themselves redefines (have a REDEFINES clause) can appear before the next segment redefine.
Closes #801
Summary by CodeRabbit
Release Notes
New Features & Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.