#774 Make Hive table existence check configurable#775
Merged
Conversation
…eCheckStrategy` for Hive table existence checks.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReplaces boolean Hive existence-check flags with ChangesExistenceCheckStrategy migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
pramen/core/src/main/scala/za/co/absa/pramen/core/utils/hive/ExistenceCheckStrategy.scala (1)
19-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
ExistenceCheckStrategya sealed trait for compile-time exhaustiveness.All four case objects are defined in this same file, so sealing the trait is trivial. Without
sealed, the pattern match inQueryExecutorJdbc.doesTableExist(lines 40–53) is not exhaustiveness-checked — adding a new strategy elsewhere would cause a runtimeMatchErrorinstead of a compile-time error.♻️ Proposed change
-sealed trait ExistenceCheckStrategy +sealed trait ExistenceCheckStrategy🤖 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/hive/ExistenceCheckStrategy.scala` around lines 19 - 40, Make ExistenceCheckStrategy a sealed trait so the compiler can verify exhaustiveness across the case objects defined in ExistenceCheckStrategy and the pattern match in QueryExecutorJdbc.doesTableExist. Update the trait declaration to sealed, keeping the existing case objects and fromString logic unchanged, so any future strategy additions must be handled at compile time instead of risking a MatchError at runtime.pramen/core/src/test/scala/za/co/absa/pramen/core/metastore/model/HiveConfigSuite.scala (1)
56-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the
optimize.exist.query = false→SelectQueryfallback path.The existing "overridden config" test sets both
optimize.exist.query = falseandtable.existence.check.strategy = "describe_table", which verifies strategy-over-boolean precedence. However, no test covers the case where onlyoptimize.exist.query = falseis set without a strategy — the key backward-compatibility path that should yieldSelectQuery.🧪 Suggested test case
"return SelectQuery when optimize.exist.query is false and no strategy is specified" in { val conf = ConfigFactory.parseString( """conf { | optimize.exist.query = false |} |""".stripMargin) val defaultConfig = HiveDefaultConfig( HiveApi.Sql, Some("mydb1"), Map("parquet" -> HiveQueryTemplates("create1", "create_only1", "replace1", "replace_part1", "repair1", "add_partition1", "drop1")), None, ignoreFailures = false, alwaysEscapeColumnNames = false, optimizeExistQuery = true, tableExistenceCheckStrategy = None ) val hiveConfig = HiveConfig.fromConfigWithDefaults(conf, defaultConfig, DataFormat.Parquet("dummy")) assert(hiveConfig.existenceCheckStrategy == ExistenceCheckStrategy.SelectQuery) }🤖 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/metastore/model/HiveConfigSuite.scala` around lines 56 - 110, Add a focused test in HiveConfigSuite for the backward-compatibility path where only conf.optimize.exist.query is set to false and no table.existence.check.strategy is provided; use HiveConfig.fromConfigWithDefaults and assert that the resulting existenceCheckStrategy resolves to ExistenceCheckStrategy.SelectQuery. Keep the existing overridden config test as-is since it already covers strategy precedence, and add a separate test case near it so the fallback behavior is explicitly validated.
🤖 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/hive/ExistenceCheckStrategy.scala`:
- Around line 19-40: Make ExistenceCheckStrategy a sealed trait so the compiler
can verify exhaustiveness across the case objects defined in
ExistenceCheckStrategy and the pattern match in
QueryExecutorJdbc.doesTableExist. Update the trait declaration to sealed,
keeping the existing case objects and fromString logic unchanged, so any future
strategy additions must be handled at compile time instead of risking a
MatchError at runtime.
In
`@pramen/core/src/test/scala/za/co/absa/pramen/core/metastore/model/HiveConfigSuite.scala`:
- Around line 56-110: Add a focused test in HiveConfigSuite for the
backward-compatibility path where only conf.optimize.exist.query is set to false
and no table.existence.check.strategy is provided; use
HiveConfig.fromConfigWithDefaults and assert that the resulting
existenceCheckStrategy resolves to ExistenceCheckStrategy.SelectQuery. Keep the
existing overridden config test as-is since it already covers strategy
precedence, and add a separate test case near it so the fallback behavior is
explicitly validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96125971-3132-4b92-9013-3e50e46600cd
📒 Files selected for processing (10)
pramen/core/src/main/scala/za/co/absa/pramen/core/metastore/model/HiveConfig.scalapramen/core/src/main/scala/za/co/absa/pramen/core/metastore/model/HiveDefaultConfig.scalapramen/core/src/main/scala/za/co/absa/pramen/core/utils/hive/ExistenceCheckStrategy.scalapramen/core/src/main/scala/za/co/absa/pramen/core/utils/hive/HiveHelper.scalapramen/core/src/main/scala/za/co/absa/pramen/core/utils/hive/QueryExecutorJdbc.scalapramen/core/src/test/scala/za/co/absa/pramen/core/metastore/model/HiveConfigSuite.scalapramen/core/src/test/scala/za/co/absa/pramen/core/metastore/model/MetaTableSuite.scalapramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/hive/ExistenceCheckStrategySuite.scalapramen/core/src/test/scala/za/co/absa/pramen/core/tests/utils/hive/QueryExecutorJdbcSuite.scalapramen/extras/src/main/scala/za/co/absa/pramen/extras/sink/StandardizationSink.scala
Unit Test Coverage
Files
|
….query = false and seal ExistenceCheckStrategy trait.
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
New Features
table.existence.check.strategyto choose how Hive checks table existence (metadata-based, describe-based, combined fallback, or SQL-query-based), with defaults applied when unset.Bug Fixes
Tests