#763 Implement relaxed type restrictions on which field can be used for record length. #764
Conversation
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
…or record length.
|
""" WalkthroughThe changes update Cobrix to support decimal fields with zero scale as valid record length fields, especially when the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CobrixReader
participant Validator
participant Extractor
User->>CobrixReader: Read data with record length field (may be Decimal)
CobrixReader->>Validator: Validate record length field type
Validator-->>CobrixReader: Log warning if not integral/decimal(scale=0), allow Decimal(scale=0)
CobrixReader->>Extractor: Extract record length
Extractor->>Extractor: Accept Integral or Decimal(scale=0), convert as needed
Extractor-->>CobrixReader: Return record length or error if invalid
CobrixReader-->>User: Data read or error
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
JaCoCo code coverage report - 'cobol-parser'
|
JaCoCo code coverage report - 'spark-cobol'
|
There was a problem hiding this comment.
Pull Request Overview
This PR relaxes type checks for record length fields, allowing zero-scale decimals and improving handling of non-integral values, while updating tests to cover these scenarios.
- Allow decimal types with scale 0 as valid record length fields
- Replace hard errors with warnings for non-integral fields and improve parsing error messages
- Add tests for numeric mappings, string-based lengths, and variable-length occurs
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test26FixLengthWithIdGeneration.scala | Added tests for string record length handling and updated temp file naming |
| spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test21VariableOccursForTextFiles.scala | Added a variable-occurs test with decimal depending fields |
| spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test37RecordLengthMappingSpec.scala | Added a test for numeric record-length mappings under strict mode |
| cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/validator/ReaderParametersValidator.scala | Switched exception to warning for non-integral length fields |
| cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/extractors/raw/FixedWithRecordLengthExprRawRecordExtractor.scala | Enhanced string parsing with Try and added BigDecimal support |
| cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/asttransform/DependencyMarker.scala | Allowed zero-scale decimals to mark record-length dependencies |
Comments suppressed due to low confidence (2)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/regression/Test26FixLengthWithIdGeneration.scala:197
- [nitpick] This test reuses the same description as an earlier one. Rename it to clearly distinguish the string-handling scenario from the EBCDIC numeric tests.
"correctly work with segment id generation option with length field" in {
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/extractors/raw/FixedWithRecordLengthExprRawRecordExtractor.scala:128
- [nitpick] The exception message ends with a period inside the quotes and includes mixed punctuation. For consistency, standardize on a format without the trailing period or match other messages in style.
case s: String => Try{ s.toInt }.getOrElse(throw new IllegalStateException(s"Record length value of the field ${lengthAST.name} must be an integral type, encountered: '$s'."))
| case i: Int => i | ||
| case l: Long => l.toInt | ||
| case s: String => Try{ s.toInt }.getOrElse(throw new IllegalStateException(s"Record length value of the field ${lengthAST.name} must be an integral type, encountered: '$s'.")) | ||
| case d: BigDecimal => d.toInt |
There was a problem hiding this comment.
The BigDecimal branch unconditionally truncates values, which can silently drop fractional parts. Consider validating that d.scale == 0 or using d.intValueExact() to ensure an error is thrown for non-zero scale.
Suggested change
| case d: BigDecimal => d.toInt | |
| case d: BigDecimal => Try { d.intValueExact() }.getOrElse(throw new IllegalStateException(s"Record length value of the field ${lengthAST.name} must be an integral type, encountered: '$d'.")) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Tested toInt vs toIntExact. The behavior of toInt is still what I want in this context. When decimals are used as record lengths teh fractional part needs to be truncated, not rounded.
.../scala/za/co/absa/cobrix/spark/cobol/source/regression/Test26FixLengthWithIdGeneration.scala
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #763
Summary by CodeRabbit
Bug Fixes
Tests