#656 Make offset types exhaustive when matching in SQL generators. #657
Conversation
WalkthroughAll SQL generator implementations receive consistent updates: a catch-all case is added to Changes
Sequence DiagramsequenceDiagram
actor User
participant Generator as SQL Generator
participant Handler as Offset Handler
rect rgb(240, 248, 255)
Note over Generator,Handler: Before: Unsupported Offset Type
User->>Generator: Request with unsupported offset
Generator->>Handler: Pattern match on offset type
Handler-->>Generator: Falls through / MatchError
Generator-->>User: Runtime error (unfriendly)
end
rect rgb(240, 255, 240)
Note over Generator,Handler: After: Explicit Error Handling
User->>Generator: Request with unsupported offset
Generator->>Handler: Pattern match on offset type
Handler->>Handler: Catch-all case triggered
Handler-->>Generator: IllegalArgumentException
Generator-->>User: Clear error message with type info
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorOracle.scala (1)
100-101: Excellent fix with consistent application across all SQL generators!The catch-all case correctly prevents
MatchErrorexceptions by throwing a clearIllegalArgumentExceptionwith the unsupported offset type included in the message. Verification confirms this pattern is consistently applied across all 11 SQL generator implementations (PostgreSQL, Oracle, Generic, MySQL, Sas, HsqlDb, Denodo, Db2, Microsoft, Hive, Databricks), ensuring uniform error handling throughout the codebase.Optional: Consider wrapping long lines for consistency.
Line 101 and similar lines in other generators are lengthy (~140 characters). For improved readability, consider wrapping:
case t => - throw new IllegalArgumentException(s"Offset type [${t.dataType.dataTypeString}] is not supported in Oracle SQL query generator.") + throw new IllegalArgumentException( + s"Offset type [${t.dataType.dataTypeString}] is not supported in Oracle SQL query generator." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDatabricks.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDb2.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDenodo.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorGeneric.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHive.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHsqlDb.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMicrosoft.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMySQL.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorOracle.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorPostgreSQL.scala(1 hunks)pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorSas.scala(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-18T08:27:21.504Z
Learnt from: yruslan
Repo: AbsaOSS/pramen PR: 611
File: pramen/core/src/main/scala/za/co/absa/pramen/core/metastore/model/MetastoreDependencyFactory.scala:29-29
Timestamp: 2025-06-18T08:27:21.504Z
Learning: In pramen/core/src/main/scala/za/co/absa/pramen/core/metastore/model/MetastoreDependencyFactory.scala, the constant variable name was changed from DATE_UNTIL_EXPR_KEY to DATE_TO_EXPR_KEY, but both constants hold the same string value "date.to". This is a variable name refactoring, not a configuration key change, so it doesn't affect backward compatibility.
Applied to files:
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHive.scalapramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorSas.scalapramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDatabricks.scalapramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorGeneric.scala
🧬 Code graph analysis (9)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDb2.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMySQL.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHsqlDb.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorPostgreSQL.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMicrosoft.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDenodo.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorOracle.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorSas.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorGeneric.scala (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/bookkeeper/model/OffsetRecords.scala (1)
dataType(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test Spark 3.3.4 on Scala 2.13.16
- GitHub Check: Test Spark 3.4.4 on Scala 2.12.20
- GitHub Check: Test Spark 2.4.8 on Scala 2.11.12
- GitHub Check: Test Spark 3.5.5 on Scala 2.13.16
- GitHub Check: Test Spark 3.5.5 on Scala 2.12.20
- GitHub Check: Test Spark 3.3.4 on Scala 2.12.20
- GitHub Check: Test Spark 3.4.4 on Scala 2.13.16
- GitHub Check: Test Coverage on Scala 2.12.18
🔇 Additional comments (10)
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorSas.scala (1)
202-204: LGTM! Excellent fix for the match exception issue.The catch-all case correctly handles unsupported
OffsetValuetypes by throwing a descriptiveIllegalArgumentExceptioninstead of letting aMatchErrorpropagate. This directly addresses issue #656 where kafka offset types would cause runtime exceptions, and the error message clearly identifies both the unsupported type and which SQL generator is rejecting it.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorPostgreSQL.scala (1)
103-104: Excellent fix for the match exception issue.This catch-all case properly addresses issue #656 by replacing the uninformative
MatchErrorwith a clear, user-friendlyIllegalArgumentExceptionthat includes the unsupported offset type. The error message provides sufficient context for debugging and troubleshooting.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHive.scala (1)
121-122: LGTM! Excellent fix for the exhaustiveness issue.The catch-all case correctly handles unsupported offset types by throwing an explicit
IllegalArgumentExceptionwith a clear error message. This replaces the previousMatchErrorwith a user-friendly exception that identifies both the unsupported type and the specific SQL generator.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDatabricks.scala (1)
121-122: LGTM! Consistent with other SQL generators.The catch-all case provides the same defensive handling as in SqlGeneratorHive and other SQL generators. The error message correctly identifies the Databricks generator and includes the unsupported offset type, ensuring users can quickly diagnose configuration issues.
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorGeneric.scala (1)
107-108: LGTM! Excellent fix for the match exhaustiveness issue.The catch-all case correctly addresses issue #656 by replacing the implicit
MatchErrorwith an explicitIllegalArgumentExceptionthat provides a user-friendly error message. This ensures that unsupported offset types (like "kafka" for JDBC sources) produce clear diagnostic information rather than cryptic match exceptions.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorHsqlDb.scala (1)
107-109: Code change is correct and safe — dataType property is guaranteed accessible.The sealed trait contract ensures all
OffsetValuesubtypes have thedataTypeproperty, eliminating any risk ofNullPointerException. The pattern is consistently applied across 11 SQL generator implementations, confirming intentional design.Consider adding test coverage for unsupported offset types (e.g., passing a
KafkaOffsetValueto HSQLDB generator) to prevent regressions.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDenodo.scala (1)
125-126: LGTM! Good fix for the match exception issue.The catch-all case correctly addresses the bug by throwing a user-friendly
IllegalArgumentExceptioninstead of allowing a match exception. The error message is clear and includes the unsupported type.Verification confirms this fix is consistently applied across all 11 SQL generators (Databricks, Db2, Generic, Hive, HsqlDb, Microsoft, MySQL, Oracle, PostgreSQL, Sas, and Denodo) with identical error handling patterns.
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMySQL.scala (1)
103-104: Good defensive fallback. The explicit IllegalArgumentException avoids a scala.MatchError and gives users actionable detail about unsupported offsets in MySQL.pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorMicrosoft.scala (1)
193-194: Verified: Consistent error handling across all SQL generators.This fix properly replaces silent
MatchErrorwith explicitIllegalArgumentExceptionacross all 10 SQL generator implementations. Each generator includes:
- Catch-all pattern
case t =>to capture the unsupported offset type- Clear error message specifying the database type (e.g., "MS SQL Server query generator", "PostgreSQL query generator")
- Access to actual type information via
t.dataType.dataTypeStringfor debuggingAll implementations are consistent and follow the same error-handling pattern. The approach provides users with actionable feedback when using unsupported offset types (e.g., Kafka) with JDBC data sources.
pramen/core/src/main/scala/za/co/absa/pramen/core/sql/SqlGeneratorDb2.scala (1)
107-108: API usage confirmed; test coverage requires manual verification.The fix is correctly implemented and consistent across all 11 SQL generators. The
dataType.dataTypeStringAPI is valid, as confirmed by existing usage in the OffsetValue class, and all 11 generators now consistently throwIllegalArgumentExceptionwith database-specific error messages for unsupported offset types.However, no tests were found in
SqlGeneratorDb2Suite.scalafor the error handling path. Please confirm that unit tests exist to validate this catch-all case with unsupported types likeKafkaValue, as their absence could leave this error path untested.
Unit Test Coverage
Files
|
Closes #656
Summary by CodeRabbit