Codecov Report

Attention: Patch coverage is 74.07407% with 35 lines in your changes missing coverage. Please review.

Project coverage is 50.22%. Comparing base (ab97ff6) to head (03ea26f).
Report is 7 commits behind head on main.

@@             Coverage Diff              @@
##               main    #1840      +/-   ##
============================================
+ Coverage     43.98%   50.22%   +6.23%     
+ Complexity     3457     1182    -2275     
============================================
  Files           827      372     -455     
  Lines         49052    20146   -28906     
  Branches       5141     2035    -3106     
============================================
- Hits          21576    10118   -11458     
+ Misses        25829     9352   -16477     
+ Partials       1647      676     -971     

@bharadwaj-aditya
I took a 2nd look at making changes to the session file based mapping logic inside bulk and live migration template.

Code ref -

1. https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/30370f0460b2a29d4266d0ef95f6ec1e06a20d9e/v2/sourcedb-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/PipelineController.java#L64

2. https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/30370f0460b2a29d4266d0ef95f6ec1e06a20d9e/v2/spanner-common/src/main/java/com/google/cloud/teleport/v2/spanner/migrations/convertors/ChangeEventSessionConvertor.java#L88

Observations -

1. Post integration of SessionFileMapper in live migration template, it would only be used to call two utility methods getSpannerTableName and getSpannerColumnName instead of accessing them directly via the schema object, which live migration template does today. This is not much benefit.
2. SessionFileMapper requires a Ddl object as input to construct it, which is not required at the place where overrides are implemented currently.
3. A large part of the mapping logic for bulk migration is actually written inside the PipelineController, similar is the case for live migrations, where it is written in ChangeEventTransformerDoFn. In the discussion that we had on greater encapsulation of the mapping logic, it seems like we will need to refactor both bulk and live migration to achieve it.

I will have to dig a bit deeper into the bulk code, but it looks like actually some of the methods like getSpannerColumnName in SessionFileMapper are actually only used in UTs. It's not immediately clear to me why.

I'll think a bit more on how we can go about this, but we should treat it as a separate exercise IMO.

@bharadwaj-aditya I took a 2nd look at making changes to the session file based mapping logic inside bulk and live migration template.

Code ref -

1. https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/30370f0460b2a29d4266d0ef95f6ec1e06a20d9e/v2/sourcedb-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/PipelineController.java#L64
2. https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/30370f0460b2a29d4266d0ef95f6ec1e06a20d9e/v2/spanner-common/src/main/java/com/google/cloud/teleport/v2/spanner/migrations/convertors/ChangeEventSessionConvertor.java#L88

Observations -

1. Post integration of SessionFileMapper in live migration template, it would only be used to call two utility methods getSpannerTableName and getSpannerColumnName instead of accessing them directly via the schema object, which live migration template does today. This is not much benefit.
2. SessionFileMapper requires a Ddl object as input to construct it, which is not required at the place where overrides are implemented currently.
3. A large part of the mapping logic for bulk migration is actually written inside the PipelineController, similar is the case for live migrations, where it is written in ChangeEventTransformerDoFn. In the discussion that we had on greater encapsulation of the mapping logic, it seems like we will need to refactor both bulk and live migration to achieve it.

I will have to dig a bit deeper into the bulk code, but it looks like actually some of the methods like getSpannerColumnName in SessionFileMapper are actually only used in UTs. It's not immediately clear to me why.

I'll think a bit more on how we can go about this, but we should treat it as a separate exercise IMO.

Regarding 1 - Agreed the ISchemaMapper does some stuff which is not required here.

Regarding 2 - Agreed - it is not required currently, but the logic to resolve it etc exists in the template in any case, any concern in using that in this flow ? It comes as the ddlView which is loaded as a part of the pipeline as opposed to launcher, but i'm not sure that is reason enough to not use it.

Regarding 3 -
In the bulk migration flow, the approach is to be SpannerSchema first, so the calls being made are getSourceColumn instead of getSpannerColumn. The mapper is being used for these calls in both identity and session.