#739 Add snapshot properties (info_date, batch_id) to Iceberg writes.#770
Merged
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughIceberg writes now attach snapshot-property metadata derived from the info-date and batch-id columns across append, overwrite, and table-creation paths. The Iceberg long suite now uses Iceberg-specific persistence construction and checks the resulting snapshot summary values. ChangesIceberg Snapshot Metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as MetastorePersistenceIcebergLongSuite
participant Persistence as MetastorePersistenceIceberg
participant Iceberg as Iceberg table
Test->>Persistence: getIcebergMtPersistence(...)
Test->>Persistence: saveTable(writeOptions)
Persistence->>Persistence: add snapshot-property.info_date and snapshot-property.pramen_batchid
Persistence->>Iceberg: append / overwrite / create with enriched options
Test->>Iceberg: currentSnapshot().summary()
Iceberg-->>Test: info_date, pramen_batchid
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 (1)
pramen/core/src/test/scala/za/co/absa/pramen/core/metastore/persistence/MetastorePersistenceIcebergLongSuite.scala (1)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate table-loading logic with
getTablePartitions.Loading the Iceberg table via
HadoopTables/Pathto inspect metadata duplicates the same logic already ingetTablePartitions(Lines 196-199). Consider extracting a sharedloadIcebergTable(tableName)helper used by both.♻️ Proposed refactor
+ def loadIcebergTable(tableName: String): Table = { + val location = new Path(hadoopTempDir, s"pramen/iceberg_catalog/default/$tableName").toString + new HadoopTables().load(location) + } + def getTablePartitions(tableName: String): Seq[String] = { - val location = new Path(hadoopTempDir, s"pramen/iceberg_catalog/default/$tableName").toString - val ht = new HadoopTables - val table = ht.load(location) + val table = loadIcebergTable(tableName) table.spec() ...And in the test:
- val location = new Path(hadoopTempDir, s"pramen/iceberg_catalog/default/$tableName").toString - val ht = new HadoopTables - val table = ht.load(location) - val snapshotProperties = table.currentSnapshot().summary().asScala + val snapshotProperties = loadIcebergTable(tableName).currentSnapshot().summary().asScala🤖 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/persistence/MetastorePersistenceIcebergLongSuite.scala` around lines 91 - 99, The test repeats Iceberg table-loading logic instead of reusing the existing approach already used by getTablePartitions. Extract a shared loadIcebergTable(tableName) helper that builds the Path, creates HadoopTables, and loads the table, then use that helper both in the snapshot-properties assertions and in getTablePartitions so the table lookup lives in one place.
🤖 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/test/scala/za/co/absa/pramen/core/metastore/persistence/MetastorePersistenceIcebergLongSuite.scala`:
- Around line 91-99: The test repeats Iceberg table-loading logic instead of
reusing the existing approach already used by getTablePartitions. Extract a
shared loadIcebergTable(tableName) helper that builds the Path, creates
HadoopTables, and loads the table, then use that helper both in the
snapshot-properties assertions and in getTablePartitions so the table lookup
lives in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8f6d786-ff70-4fd1-a7f5-ff9a8b0de00b
📒 Files selected for processing (2)
pramen/core/src/main/scala/za/co/absa/pramen/core/metastore/peristence/MetastorePersistenceIceberg.scalapramen/core/src/test/scala/za/co/absa/pramen/core/metastore/persistence/MetastorePersistenceIcebergLongSuite.scala
Unit Test Coverage
Files
|
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.
Closes #739
Summary by CodeRabbit
Bug Fixes
Tests