Add support for writing VB (variable block) COBOL files - #867
Merged
Conversation
Enable writing record_format=VB by grouping variable-length records into BDW-prefixed blocks. Blocking is controlled by 'records_per_block' or 'block_length' (mutually exclusive, one required). - CobolParametersParser: thread isWriter into parseBdw so 'block_length' is honored when writing VB (on read it is self-described by the BDW). - CobolParametersValidator: allow VB for writing and require exactly one blocking option. - NestedRecordCombiner: additive groupIntoBlocks/buildBlock stage that packs per-record (RDW+payload) arrays into BDW+records blocks per partition; oversized single record gets its own block; blocks do not span partitions. Existing F/V paths are unchanged. - Tests: new VariableBlockEbcdicWriterSuite (byte-exact + round-trip). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
WalkthroughThis change adds Variable Block ( ChangesVariable-block writer support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SparkWriter
participant CobolParametersValidator
participant NestedRecordCombiner
participant BDWOutput
SparkWriter->>CobolParametersValidator: validate VariableBlock settings
CobolParametersValidator-->>SparkWriter: accept block_length or records_per_block
SparkWriter->>NestedRecordCombiner: combine encoded RDW records
NestedRecordCombiner->>NestedRecordCombiner: group records by block size or count
NestedRecordCombiner->>BDWOutput: prepend endian-specific BDW headers
BDWOutput-->>SparkWriter: write grouped VB blocks
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/VariableBlockEbcdicWriterSuite.scala (1)
236-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a big-endian round-trip case.
The round trip only covers the default little-endian BDW. The two encodings are not symmetric: the little-endian BDW keeps the length in bytes 2-3, while the little-endian RDW keeps it in bytes 0-1. A read-back test with
is_bdw_big_endian=trueandis_rdw_big_endian=truelocks the writer and the reader decoder together for both byte orders.🤖 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/VariableBlockEbcdicWriterSuite.scala` around lines 236 - 263, Extend the round-trip test in “round-trip: write a VB file and read it back” with a big-endian case by setting both is_bdw_big_endian and is_rdw_big_endian to true for writing and reading. Preserve the existing data, ordering, and expected readBack assertions so the writer and reader are validated together for both byte orders.
🤖 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/reader/parameters/CobolParametersParser.scala`:
- Around line 640-644: Update the README options table and example text for
variable-block (VB) records to document that block_length and records_per_block
are accepted for record_format = VB, must be provided together where applicable,
and are mutually exclusive so only one may be specified.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/parameters/CobolParametersValidator.scala`:
- Around line 155-160: Update the VariableBlock validation in
CobolParametersValidator to reject any defined bdw.blockLength or
bdw.recordsPerBlock value that is zero or negative, while preserving the
existing requirement that at least one option is specified. Add tests covering
zero and negative values for both blocking options.
---
Nitpick comments:
In
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/VariableBlockEbcdicWriterSuite.scala`:
- Around line 236-263: Extend the round-trip test in “round-trip: write a VB
file and read it back” with a big-endian case by setting both is_bdw_big_endian
and is_rdw_big_endian to true for writing and reading. Preserve the existing
data, ordering, and expected readBack assertions so the writer and reader are
validated together for both byte orders.
🪄 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 Plus
Run ID: 96770b56-3974-4d18-aaae-d0e32e4d8dae
📒 Files selected for processing (6)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/parameters/CobolParametersValidator.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer/NestedRecordCombiner.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/parameters/CobolParametersValidatorSuite.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/VariableBlockEbcdicWriterSuite.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/VariableLengthEbcdicWriterSuite.scala
Comment on lines
+640
to
644
| // When reading VB the block length is self-described by the BDW header in the file, so 'block_length' is ignored. | ||
| // When writing VB, 'block_length' is meaningful: it caps how many records are packed into each block. | ||
| if (recordFormat == VariableBlock && bdw.blockLength.nonEmpty && !isWriter) { | ||
| logger.warn(s"Option '$PARAM_BLOCK_LENGTH' is ignored for record format: VB") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for documentation of VB writing and the blocking options.
fd -e md . | xargs rg -n -C3 'records_per_block|block_length|record_format.{0,10}VB'Repository: AbsaOSS/cobrix
Length of output: 6603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^README\.md$|cobol-parser/src/main/scala/.*/CobolParametersParser\.scala$|writer|cobol/writer|output)' | sed -n '1,200p'
echo
echo "== reader parser block_length warning context =="
file=$(git ls-files | rg 'cobol-parser/src/main/scala/.*/CobolParametersParser\.scala$' | head -n1)
echo "FILE=$file"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '600,660p' "$file" | nl -ba -v600
fi
echo
echo "== options table lines 1740-1775 =="
sed -n '1740,1775p' README.md | nl -ba -v1740
echo
echo "== search variable block writing mentions with broad text =="
rg -n -C3 'variable[block ]+length|VB|records_per_block|block_length|writer|write|output' README.md cobol-parser/src/main/scala za || trueRepository: AbsaOSS/cobrix
Length of output: 1503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scala"
echo "== reader parser block_length warning context =="
wc -l "$file"
sed -n '615,650p' "$file"
echo
echo "== write path references to block_length/records_per_block/VariableBlock =="
rg -n -C4 'blockLength|recordsPerBlock|block_length|records_per_block|VariableBlock|block_length|records_per_block|records_per_block|block_length' spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/writer README.md | sed -n '1,240p'
echo
echo "== options table lines 1740-1775 =="
sed -n '1740,1775p' README.md
echo
echo "== sample variable-block test names/descriptions =="
rg -n -C3 'variable|VB|blockLength|recordsPerBlock|records_per_block|block_length' spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/writer/VariableBlockEbcdicWriterSuite.scala | sed -n '1,220p'Repository: AbsaOSS/cobrix
Length of output: 32804
Document the VB writer blocking behavior.
block_length and records_per_block both drive VB output, and both are required together for reading and exclusion, but README.md still lists them only under FB and omits the variable-record VB section. Update the options table/example text so users know either option is accepted for record_format = VB and only one may be specified.
🤖 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/reader/parameters/CobolParametersParser.scala`
around lines 640 - 644, Update the README options table and example text for
variable-block (VB) records to document that block_length and records_per_block
are accepted for record_format = VB, must be provided together where applicable,
and are mutually exclusive so only one may be specified.
Comment on lines
+155
to
+160
| if (readerParameters.recordFormat == RecordFormat.VariableBlock) { | ||
| val hasBlockLength = readerParameters.bdw.exists(_.blockLength.nonEmpty) | ||
| val hasRecordsPerBlock = readerParameters.bdw.exists(_.recordsPerBlock.nonEmpty) | ||
| if (!hasBlockLength && !hasRecordsPerBlock) { | ||
| issues += "Writing 'VB' records requires either 'records_per_block' or 'block_length' to be specified" | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-positive VB blocking values.
This branch checks only option presence. It accepts block_length <= 0 and records_per_block <= 0. The parser converts these options directly to Int without a range check. (raw.githubusercontent.com) The downstream combiner treats records_per_block as an exact count, so these values do not describe a valid block configuration. (raw.githubusercontent.com)
Validate each defined value as positive and add tests for zero and negative values.
Proposed fix
if (readerParameters.recordFormat == RecordFormat.VariableBlock) {
+ readerParameters.bdw.foreach { bdw =>
+ bdw.blockLength.foreach { value =>
+ if (value <= 0) {
+ issues += "'block_length' must be a positive integer"
+ }
+ }
+ bdw.recordsPerBlock.foreach { value =>
+ if (value <= 0) {
+ issues += "'records_per_block' must be a positive integer"
+ }
+ }
+ }
val hasBlockLength = readerParameters.bdw.exists(_.blockLength.nonEmpty)
val hasRecordsPerBlock = readerParameters.bdw.exists(_.recordsPerBlock.nonEmpty)📝 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.
Suggested change
| if (readerParameters.recordFormat == RecordFormat.VariableBlock) { | |
| val hasBlockLength = readerParameters.bdw.exists(_.blockLength.nonEmpty) | |
| val hasRecordsPerBlock = readerParameters.bdw.exists(_.recordsPerBlock.nonEmpty) | |
| if (!hasBlockLength && !hasRecordsPerBlock) { | |
| issues += "Writing 'VB' records requires either 'records_per_block' or 'block_length' to be specified" | |
| } | |
| if (readerParameters.recordFormat == RecordFormat.VariableBlock) { | |
| readerParameters.bdw.foreach { bdw => | |
| bdw.blockLength.foreach { value => | |
| if (value <= 0) { | |
| issues += "'block_length' must be a positive integer" | |
| } | |
| } | |
| bdw.recordsPerBlock.foreach { value => | |
| if (value <= 0) { | |
| issues += "'records_per_block' must be a positive integer" | |
| } | |
| } | |
| } | |
| val hasBlockLength = readerParameters.bdw.exists(_.blockLength.nonEmpty) | |
| val hasRecordsPerBlock = readerParameters.bdw.exists(_.recordsPerBlock.nonEmpty) | |
| if (!hasBlockLength && !hasRecordsPerBlock) { | |
| issues += "Writing 'VB' records requires either 'records_per_block' or 'block_length' to be specified" | |
| } |
🤖 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/source/parameters/CobolParametersValidator.scala`
around lines 155 - 160, Update the VariableBlock validation in
CobolParametersValidator to reject any defined bdw.blockLength or
bdw.recordsPerBlock value that is zero or negative, while preserving the
existing requirement that at least one option is specified. Add tests covering
zero and negative values for both blocking options.
yruslan
approved these changes
Aug 4, 2026
yruslan
left a comment
yruslan
left a comment
Collaborator
There was a problem hiding this comment.
Fantastic! Looks great, thank you for your contribution!
Contributor
Author
|
Thank you @yruslan ! Soon I'll open also the PR for REDEFINES management. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hi @yruslan, I'm back again to this project with another contribution that I want to submit to your attention, this PR:
Adds the ability to write VB (variable block) COBOL files with the Spark writer. Previously
record_format=VBwas supported for reading only; writing rejected anything other thanF/V.A VB file groups one or more variable-length records into blocks:
that follows it in the block (sum of all
RDW + payloadbytes), excluding the BDW's own 4 bytes —mirroring the existing reader convention so files round-trip.
How blocking is controlled
Writing VB requires exactly one of the following (mutually exclusive; one is mandatory):
records_per_blockblock_lengthEndianness/adjustment reuse the existing read-side options:
is_bdw_big_endian,bdw_adjustment(and
is_rdw_big_endian,rdw_adjustment,is_rdw_part_of_record_lengthfor the inner RDWs).If neither blocking option is provided, writing fails fast with a clear error rather than assuming
a silent default.
What changed
CobolParametersParser— thread the existingisWriterflag intoparseVariableLengthParameters/parseBdw. On read, VB block length is self-described by the file'sBDW header, so
block_lengthis ignored/warned; on write it is meaningful (the packing cap), so thewarning is suppressed for writers.
CobolParametersValidator.validateParametersForWriting— allowrecord_format=VBand requireexactly one of
records_per_block/block_length.NestedRecordCombiner— new additivegroupIntoBlocks/buildBlockstage that runs onlyfor VB, after the existing per-record encoding. Each record is still independently rendered to an
Array[Byte]ofRDW + payload(unchanged); the new stage packs consecutive records into blockswithin each partition and prepends the BDW. Existing
F/Vcode paths are untouched.Design decisions
writer's existing one-output-file-per-partition behavior.
block_lengthforms its own single-recordblock.
RawBinaryOutputFormatalready writes byte arrays back-to-back with noseparators, so producing one array per block is sufficient.
Testing
New suite
VariableBlockEbcdicWriterSuite(byte-exact assertions + a write→read round-trip):records_per_block=2, little-endian BDW/RDWrecords_per_block=2, big-endian BDW/RDWrecords_per_block=2with an uneven number of records (partial last block)block_lengthcap producing an uneven splitblock_length(gets its own block, not split)Regression:
sparkCobol/test593/593 passing (2 pre-existing ignored),cobolParser/test711/711 passing. Two existing tests that asserted the old "only F and V" validator message were
updated for the new behavior.
Files changed
cobol-parser/.../reader/parameters/CobolParametersParser.scalaspark-cobol/.../source/parameters/CobolParametersValidator.scalaspark-cobol/.../writer/NestedRecordCombiner.scalaspark-cobol/.../writer/VariableBlockEbcdicWriterSuite.scala(new)spark-cobol/.../source/parameters/CobolParametersValidatorSuite.scala(test update)spark-cobol/.../writer/VariableLengthEbcdicWriterSuite.scala(test update)Final 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
I'm also working on another feature for the writer: you can find more details here -> support for REDEFINES clauses in the writer. It is a feature independent to this one that could be analyzed afterwards since I like to keep PRs atomic, but that I'd like to bring to the library too.
Thanks again for the immense work you're doing into maintaining this repo.
Talk soon,
Francesco
Summary by CodeRabbit
New Features
Bug Fixes