#746 Remove CHAR(n) support from data types supported when creting Hive tables.#747
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR removes CHAR(n) support from JDBC-sourced Spark metadata and standardizes on VARCHAR(n). String-length constants are updated to increase the maximum from 255 to 8192 bytes, type resolution logic is modified to map CHAR metadata to VARCHAR, imports are adjusted, and tests are updated to validate the new mapping behavior. ChangesCHAR to VARCHAR Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
pramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/SparkUtilsSuite.scala (1)
706-713: ⚡ Quick winUpdate test name to reflect VARCHAR behavior.
The test name says "return char type from CHAR_VARCHAR_METADATA_KEY with char" but the test now expects
VarcharType(12)instead ofCharType(12). Update the test name to accurately describe that CHAR metadata is now mapped to VarcharType.📝 Proposed test name fix
- "return char type from CHAR_VARCHAR_METADATA_KEY with char" in { + "return varchar type from CHAR_VARCHAR_METADATA_KEY with char" in { val metadata = new MetadataBuilder metadata.putString(CHAR_VARCHAR_METADATA_KEY, "CHAR(12)") val stringType = SparkUtils.getStringTypeFromMetadata(metadata.build()) assert(stringType == VarcharType(12)) }🤖 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 `@pramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/SparkUtilsSuite.scala` around lines 706 - 713, Rename the failing test's description in SparkUtilsSuite: change the test name string that currently reads "return char type from CHAR_VARCHAR_METADATA_KEY with char" to something that reflects VARCHAR mapping (e.g., "return varchar type from CHAR_VARCHAR_METADATA_KEY with char") so it matches the assertion that SparkUtils.getStringTypeFromMetadata(metadata.build()) returns VarcharType(12); reference the test in SparkUtilsSuite and the CHAR_VARCHAR_METADATA_KEY constant to locate and update the description only.pramen/core/src/main/scala/za/co/absa/pramen/core/utils/SparkUtils.scala (1)
420-431: ⚡ Quick winUpdate documentation to reflect removal of CharType support.
The method documentation still mentions
CharTypeas a possible return type, but the implementation now only returnsVarcharTypeorStringType(neverCharType). Update the doc comment to remove references toCharType.📝 Proposed documentation fix
/** - * Extracts a string-based data type (CharType, VarcharType, or StringType) from field metadata. + * Extracts a string-based data type (VarcharType or StringType) from field metadata. * * First checks for the presence of character/varchar type metadata key. If found, parses the metadata - * value using a pattern to determine if it represents a CHAR or VARCHAR type with a specific length. + * value using a pattern to extract the length. Both CHAR and VARCHAR metadata are mapped to VarcharType. * If the metadata key is not present, attempts to extract a length value from metadata and creates * a VarcharType if successful. Falls back to StringType if no specific type information is found * or if the metadata cannot be parsed. * * `@param` metadata the metadata object containing type information for a field - * `@return` the resolved string-based DataType, which can be CharType, VarcharType, or StringType + * `@return` the resolved string-based DataType, which can be VarcharType or StringType */🤖 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 `@pramen/core/src/main/scala/za/co/absa/pramen/core/utils/SparkUtils.scala` around lines 420 - 431, Update the ScalaDoc for the string-type extractor in SparkUtils.scala to remove references to CharType and reflect current behavior: state that the method (e.g., extractStringBasedDataType) returns either VarcharType or StringType only, describe how metadata keys are parsed to produce a VarcharType with length or fall back to StringType, and remove any mention that CharType may be returned.
🤖 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.
Nitpick comments:
In `@pramen/core/src/main/scala/za/co/absa/pramen/core/utils/SparkUtils.scala`:
- Around line 420-431: Update the ScalaDoc for the string-type extractor in
SparkUtils.scala to remove references to CharType and reflect current behavior:
state that the method (e.g., extractStringBasedDataType) returns either
VarcharType or StringType only, describe how metadata keys are parsed to produce
a VarcharType with length or fall back to StringType, and remove any mention
that CharType may be returned.
In
`@pramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/SparkUtilsSuite.scala`:
- Around line 706-713: Rename the failing test's description in SparkUtilsSuite:
change the test name string that currently reads "return char type from
CHAR_VARCHAR_METADATA_KEY with char" to something that reflects VARCHAR mapping
(e.g., "return varchar type from CHAR_VARCHAR_METADATA_KEY with char") so it
matches the assertion that
SparkUtils.getStringTypeFromMetadata(metadata.build()) returns VarcharType(12);
reference the test in SparkUtilsSuite and the CHAR_VARCHAR_METADATA_KEY constant
to locate and update the description only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 554d823d-8ad1-4017-8dec-05e5a93f3a5b
📒 Files selected for processing (3)
pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcSparkUtils.scalapramen/core/src/main/scala/za/co/absa/pramen/core/utils/SparkUtils.scalapramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/SparkUtilsSuite.scala
💤 Files with no reviewable changes (1)
- pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcSparkUtils.scala
Unit Test Coverage
Files
|
1 participant
Closes #746
Summary by CodeRabbit