Feature: Reworked record combiner to support redefines in the writer - #869
Conversation
WalkthroughThis PR adds COBOL ChangesREDEFINES writer support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InputRow
participant NestedRecordCombiner
participant RedefineGroup
participant OutputBytes
InputRow->>NestedRecordCombiner: Provide field values
NestedRecordCombiner->>RedefineGroup: Inspect alternatives
RedefineGroup->>RedefineGroup: Detect populated alternatives
alt One alternative populated
RedefineGroup->>OutputBytes: Serialize selected alternative
else No alternative populated
RedefineGroup->>OutputBytes: Write empty shared bytes
else Multiple alternatives populated
RedefineGroup->>NestedRecordCombiner: Report conflict or select first alternative
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/WriterAst.scala (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the
actualSizedescription.The doc says the shared region size is "uniform across all alternatives". Alternatives can have different sizes. The differing-size test in
FixedLengthEbcdicWriterSuite.scala(lines 720-753) expects the region to span the widest alternative. DescribeactualSizeas the size of the shared region, which covers the widest alternative.📝 Proposed documentation fix
* `@param` alternatives The list of mutually exclusive alternatives sharing the byte region. - * `@param` actualSize The size, in bytes, of the shared byte region (uniform across all alternatives). + * `@param` actualSize The size, in bytes, of the shared byte region. It spans the widest + * alternative; narrower alternatives leave the trailing bytes as zeroes. */🤖 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 `@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/WriterAst.scala` around lines 78 - 81, Update the RedefineGroup actualSize Scaladoc to describe it as the size of the shared byte region covering the widest alternative, rather than implying all alternatives have uniform sizes.spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scala (1)
566-574: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a deep population check for group alternatives.
isPopulatedtreats aGroupFieldas populated when the nestedRowis not null. A row can contain a non-null struct whose fields are all null. Two alternatives can then both look populated, and the write fails with the conflict error even though no value exists.A recursive check over children would make the decision match the actual data:
♻️ Proposed deep check for group nodes
- case GroupField(_, _, getter) => getter(row) != null + case GroupField(children, _, getter) => + val nestedRow = getter(row) + nestedRow != null && children.exists(child => isPopulated(child, nestedRow))Confirm the intended semantics before applying this change. Spark JSON sources usually produce a null struct for an absent group, so the current check is sufficient for the added tests.
🤖 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 `@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scala` around lines 566 - 574, Confirm the intended population semantics before changing isPopulated: Spark JSON inputs typically represent absent groups as null structs, so retain the current non-null GroupField and GroupArray checks unless the added tests require distinguishing empty nested Rows. Do not introduce a recursive child-value check without validation, while preserving RedefineGroup alternative conflict behavior.
🤖 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
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala`:
- Around line 574-576: Strengthen both REDEFINES conflict assertions in
FixedLengthEbcdicWriterSuite: at
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala:574-576,
require m.contains("'B', 'B1'"); at
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala:668-670,
require m.contains("'B', 'B2'") instead of separate substring checks.
---
Nitpick comments:
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scala`:
- Around line 566-574: Confirm the intended population semantics before changing
isPopulated: Spark JSON inputs typically represent absent groups as null
structs, so retain the current non-null GroupField and GroupArray checks unless
the added tests require distinguishing empty nested Rows. Do not introduce a
recursive child-value check without validation, while preserving RedefineGroup
alternative conflict behavior.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/WriterAst.scala`:
- Around line 78-81: Update the RedefineGroup actualSize Scaladoc to describe it
as the size of the shared byte region covering the widest alternative, rather
than implying all alternatives have uniform sizes.
🪄 Autofix
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 Plus
Run ID: d7473955-71db-4ddb-aaf2-0c94cc2ed711
📒 Files selected for processing (3)
spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/WriterAst.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala
| val messages = causeChainMessages(thrown) | ||
| assert(messages.exists(m => m.contains("B") && m.contains("B1")), | ||
| s"Expected an error mentioning both conflicting REDEFINES fields 'B' and 'B1', but got: ${messages.mkString(" | ")}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both REDEFINES conflict assertions accept a message that names only one field. The shared root cause is a substring predicate: "B1" and "B2" both contain "B", so m.contains("B") adds no verification. Assert the exact conflicting-name list that the writer produces.
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala#L574-L576: replace the twocontainschecks withm.contains("'B', 'B1'").spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala#L668-L670: replace the twocontainschecks withm.contains("'B', 'B2'").
📍 Affects 1 file
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala#L574-L576(this comment)spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala#L668-L670
🤖 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
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala`
around lines 574 - 576, Strengthen both REDEFINES conflict assertions in
FixedLengthEbcdicWriterSuite: at
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala:574-576,
require m.contains("'B', 'B1'"); at
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scala:668-670,
require m.contains("'B', 'B2'") instead of separate substring checks.
yruslan
left a comment
yruslan
left a comment
There was a problem hiding this comment.
This is amazing! The solution is very elegant and solves the very important use case. I like it a lot. Have just 1 suggestion to consider.
| case multiple => | ||
| val fieldNames = multiple.map(_.fieldName).mkString("', '") | ||
| throw new IllegalArgumentException( | ||
| s"Conflicting REDEFINES fields populated on the same row: '$fieldNames'. " + | ||
| s"Only one field of a REDEFINES group can have a non-null value at a time." | ||
| ) |
There was a problem hiding this comment.
Throwing exceptions from inside a Spark job is not a usual practice since this can cancel a job that processes GBs of data just on a single data error. Usually, in Spark throwing exception on data is the last resort.
I'd prefer when multiple alternatives are possible, just use the first one.
No need to fix it yourself, I can fix the logic once the PR is merged. Up to you.
There was a problem hiding this comment.
Yep, I totally agree with you.
I can work on this between today and tomorrow and update the PR by implementing your suggestion (use the first one when there there are multiple alternatives).
There was a problem hiding this comment.
Ciao @yruslan , I've updated the PR with your suggestion.
I modeled it with a new writer option that
- by default is non-strict: when multiple alternatives are possible, takes the first one.
- eventually can be made strict: fails when multiple alternatives are found.
I did that because I agreed with you that from an analytics point of view throwing GBs of data for an error on a single row is not optimal.
However, from an operational point of view, an aware user, could opt for a stricter management of such scenarios to fail the job to avoid unwanted writes during the process that can potentially harm downstream operations.
I've updated the PR text/references accordingly.
Let me know what you think about this.
There was a problem hiding this comment.
Perfect, thank you! Merging...
One additional use case came to mind in regards to redefines. In come copybooks we have redefines that provide multiple 'views' on the same field, e.g.:
05 ACCOUNT-NUMBER-FULL 9(10).
05 ACCOUNT-NUMBER-DETAIL REDEFINES ACCOUNT-NUMBER-FULL.
10 PREFIX 9(4).
10 NUMBER 9(6).In this case both alternatives would have values, but they are essentially the same. So having the relaxed redefine strictness by default makes perfect sense.
2 participants
Summary
This PR adds
REDEFINESsupport toNestedRecordCombiner. Previously, the writer silently dropped any field declared withREDEFINESwhen building its internal AST (stmt.redefines.isEmptyfilter), meaning DataFrames could only ever populate the base field of a redefined region — any attempt to write via a redefining field was ignored or failed under strict schema validation.What changed
WriterAst.scalaRedefineAlternative(fieldName, ast)andRedefineGroup(alternatives, actualSize)AST node types to represent a set of mutually-exclusive fields sharing the same byte region.NestedRecordCombiner.scalabuildGroupFieldnow clusters a base field together with its consecutive chain ofREDEFINESfields into a singleRedefineGroupnode (mirroring the clustering/sizing already done incobol-parser'sBinaryPropertiesAdder), instead of dropping the redefining fields.buildRedefineGroup/buildChildNodehelpers build each alternative (primitive or nested group) leniently, then apply strict-schema validation once at the cluster level — failing only if none of the alternatives are present in the DataFrame schema.writeToBytesgained aRedefineGroupcase:write_strict_redefinesoption (defaultfalse):write_strict_redefines=true): fails fast with a clearIllegalArgumentExceptionnaming the conflicting fields, as before.isPopulatedhelper determines whether an AST node (primitive, group, or nestedRedefineGroup) has a value for a given row.Behavior notes / design decisions
write_strict_redefines=true.0x00), not spaces.cobol-parser,RecordCombinerSelector, or reading/decoding logic — this is writer-only and fully backward compatible with non-REDEFINES copybooks.Testing
Added new tests to
FixedLengthEbcdicWriterSuite:write_strict_redefines=true.write_strict_redefines=true.strict_schema=false.strict_schema=true(default).REC-TYPEdiscriminator column following the common COBOL convention for tagging which alternative a record uses.Also added a
CobolParametersParserSuitecase verifyingwrite_strict_redefinesparses intoWriterParameters.strictRedefines.Files changed
spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/WriterAst.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/FixedLengthEbcdicWriterSuite.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/WriterParameters.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParserSuite.scalaFinal Notes
Please let me know what do you think about this PR and if there is the margin of adding this functionality to the library. I'm open to further communication and collaboration and looking forward to read feedbacks from you.
Co-author of this PR: Andrea Fonti
Thanks again for the immense work you're doing into maintaining this project.
Talk soon,
Francesco
Summary by CodeRabbit
Release Notes
Improvements
Tests