#857 Disable indexes when record_limit is below threshold and pass recordLimit to VRLRecordReader. - #861
Merged
Merged
Conversation
…cordLimit to VRLRecordReader.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds end-to-end ChangesRecord limit flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CobolParametersParser
participant ReaderParameters
participant VarLenHierarchicalIterator
participant VRLRecordReader
CobolParametersParser->>ReaderParameters: pass recordLimit
ReaderParameters->>VarLenHierarchicalIterator: provide readerProperties.recordLimit
VarLenHierarchicalIterator->>VRLRecordReader: construct with recordLimit
VRLRecordReader->>VRLRecordReader: stop fetching at the limit
Possibly related PRs
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 |
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReaderSpec.scala (1)
355-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread recordLimit through the test helper.
getUseCasehardcodesNone, so the VRLRecordReaderSpec tests never exercise the new limit path. AddrecordLimit: Option[Int] = Noneto the helper and cover zero, exact-N, and boundary cases.🤖 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/test/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReaderSpec.scala` around lines 355 - 356, Update the getUseCase test helper to accept a recordLimit: Option[Int] = None parameter and pass it through instead of hardcoding None. Extend VRLRecordReaderSpec coverage for zero, exact-N, and boundary record-limit cases so the new limit path is exercised.
🤖 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/iterator/VarLenHierarchicalIterator.scala`:
- Line 57: Move record-limit enforcement out of the raw-record readers and apply
it at decoded-row boundaries: in VarLenHierarchicalIterator.scala (line 57),
allow all segments of each root record to be extracted before counting the
completed hierarchical row; in VarLenNestedIterator.scala (line 55), apply the
limit only after segment filtering so filtered raw records do not consume the
decoded-row budget.
In
`@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReader.scala`:
- Around line 77-78: The record-limit guard in VRLRecordReader’s fetchNext path
uses recordIndex before it is incremented, allowing prefetch beyond the
configured limit. Track the next emitted record index separately, check the
limit before fetching, and clear the cached record when the limit is reached so
recordLimit 0 yields no rows and positive N yields exactly N rows.
---
Nitpick comments:
In
`@cobol-parser/src/test/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReaderSpec.scala`:
- Around line 355-356: Update the getUseCase test helper to accept a
recordLimit: Option[Int] = None parameter and pass it through instead of
hardcoding None. Extend VRLRecordReaderSpec coverage for zero, exact-N, and
boundary record-limit cases so the new limit path is exercised.
🪄 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: 1dbd664a-f69c-4ca9-8038-7c004ac7f67b
📒 Files selected for processing (11)
README.mdcobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReader.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenHierarchicalIterator.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenNestedIterator.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReaderSpec.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/SparkCobolProcessor.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/DefaultSource.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test43RecordLimitSpec.scala
| type RawRecord = (String, Array[Byte]) | ||
|
|
||
| private val rawRecordIterator = new VRLRecordReader(cobolSchema, dataStream, readerProperties, recordHeaderParser, rawRecordExtractor, startRecordId, startingFileOffset) | ||
| private val rawRecordIterator = new VRLRecordReader(cobolSchema, dataStream, readerProperties, recordHeaderParser, rawRecordExtractor, startRecordId, startingFileOffset, readerProperties.recordLimit) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Apply the output limit at decoded-row boundaries, not raw-record boundaries.
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenHierarchicalIterator.scala#L57-L57: enforce the limit after complete hierarchical rows are extracted; do not truncate a root record’s segments.cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenNestedIterator.scala#L55-L55: enforce the limit after segment filtering so filtered raw records do not consume the decoded-row budget.
📍 Affects 2 files
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenHierarchicalIterator.scala#L57-L57(this comment)cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenNestedIterator.scala#L55-L55
🤖 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/iterator/VarLenHierarchicalIterator.scala`
at line 57, Move record-limit enforcement out of the raw-record readers and
apply it at decoded-row boundaries: in VarLenHierarchicalIterator.scala (line
57), allow all segments of each root record to be extracted before counting the
completed hierarchical row; in VarLenNestedIterator.scala (line 55), apply the
limit only after segment filtering so filtered raw records do not consume the
decoded-row budget.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/iterator/VRLRecordReader.scala`:
- Line 87: Update the extractor-record handling in VRLRecordReader so
recordNextToFetch is incremented exactly once for each accepted payload. Remove
the duplicate increment across the paths around extractor.next() and the later
accepted-record handling, while preserving the configured record limit and
non-extractor behavior.
🪄 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: 8e3fadaa-7146-482e-a593-967f33b40861
📒 Files selected for processing (1)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReader.scala
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/iterator/VarLenHierarchicalIterator.scala`:
- Around line 102-105: Update the record-limit early-return branch in
VarLenHierarchicalIterator to clear cachedValue before closing dataStream and
returning. Ensure cachedValue is set to the empty Option state so hasNext() does
not dereference an uninitialized value or repeatedly expose the final cached
row.
🪄 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: ca4f087c-4221-416c-9acb-265d536f9dec
📒 Files selected for processing (3)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReader.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VarLenHierarchicalIterator.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test43RecordLimitSpec.scala
🚧 Files skipped from review as they are similar to previous changes (2)
- cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/iterator/VRLRecordReader.scala
- spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test43RecordLimitSpec.scala
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
1 participant
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 by CodeRabbit
record_limitsupport to cap decoded output rows for variable-length reads, including nested and hierarchical iteration.record_limit = 0returns no rows; omitting the limit reads all available records.record_limitis present and<= 100,000, indexing is automatically disabled for bounded reads.