[Data Validation] Add Primary Key IT - #4077
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4077 +/- ##
=============================================
+ Coverage 34.36% 62.53% +28.17%
- Complexity 522 3332 +2810
=============================================
Files 216 574 +358
Lines 13239 33685 +20446
Branches 1311 3696 +2385
=============================================
+ Hits 4549 21066 +16517
- Misses 8323 11528 +3205
- Partials 367 1091 +724
🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive integration testing for the GCS to Spanner data validation pipeline, specifically focusing on handling complex primary key configurations. It ensures that the pipeline correctly matches records when primary keys undergo transformations, differ between source and destination schemas, or utilize complex Avro datatypes. Additionally, a minor fix was applied to the record conversion logic to improve resilience during custom transformations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test suite, GCSSpannerDVPrimaryKeyIT, along with its Spanner schema to verify complex primary key handling in the GCSSpannerDV pipeline, covering transformed primary keys, modified primary key columns, and complex Avro datatypes. Additionally, it adds a null check in GenericRecordTypeConvertor to prevent potential null pointer exceptions when custom transformations return null. The review feedback points out that using an Integer instead of a Long for the role_id field in the integration test can cause a ClassCastException during Avro serialization, and suggests using 1L instead.
There was a problem hiding this comment.
Code Review
This pull request adds integration tests for complex primary key handling in the GCSSpannerDV pipeline and introduces a null check in GenericRecordTypeConvertor to prevent NullPointerExceptions when custom transformations return a null response row. The feedback suggests adding a 20-second delay in the testTransformedPrimaryKey integration test to prevent test flakiness due to Spanner's exact staleness read bound.
| .set("full_name") | ||
| .to("416c696365") // Hex for "Alice" |
There was a problem hiding this comment.
Nit: Is a hex transform is happening lets just call the column name appropriately, why full_name? Maybe user_id_in_hex or something?
There was a problem hiding this comment.
because i was re-using the pre-defined USERS schema it had to be full_name as i didn't want to do column renaming in this test.
But in that spirit, i have changed the logic to use a different table_name so there is a dedicated transformation for this test
| if (transformedCols == null) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The next line includes transformedCols.entrySet() which throws NullPointerException if transformedCols=null
Custom Transformation can return null in a lot of cases where a particular table is not transforming any rows and the code then fails with NullPointerException
2 participants
Call out:
Three test cases:
The first one uses a transformation, and as seen in this PR, this needs to be run on DataflowRunner to avoid resource leaks. The rest of the test methods are functional and so have been configured to run on DirectRunner