Improve oracle support in SMT - #1393
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1393 +/- ##
==========================================
+ Coverage 51.19% 51.28% +0.09%
==========================================
Files 202 203 +1
Lines 22797 22980 +183
Branches 552 552
==========================================
+ Hits 11671 11786 +115
- Misses 10438 10499 +61
- Partials 688 695 +7
🚀 New features to boost your workflow:
|
bharadwaj-aditya
left a comment
bharadwaj-aditya
left a comment
There was a problem hiding this comment.
Largely looks fine. But it looks like the data migration code has been deleted. Lets not delete that code in this PR - unless it is causing major issues in the implementation
a810045 to
9b25cc3
Compare
bharadwaj-aditya
left a comment
bharadwaj-aditya
left a comment
There was a problem hiding this comment.
Looks a lot cleaner. Some questions added here.
| ignored := schema.Ignored{} | ||
| for _, c := range constraints[colName] { | ||
| // Type of constraint definition in oracle C (check constraint on a table) | ||
| // P (primary key), U (unique key) ,R (referential integrity), V (with check option, on a view) |
There was a problem hiding this comment.
can you check if unique keys and primary keys are being identified correctly in the schema conversion ? I don't quite understand how this logic is being translated in the new implementation
There was a problem hiding this comment.
Yes unique keys are being identified from getIndexsBatch and primaryKey from getConstraintsBatch.
| return result, nil | ||
| } | ||
|
|
||
| // GetIndexes return a list of all indexes for the specified table. |
There was a problem hiding this comment.
please retain the comments where relevant.
Are we handling all these index types in the conversion ?
There was a problem hiding this comment.
Added comments on top of critical methods on what it does.
Are we handling all these index types in the conversion ?
Yes we are handling it, below is the mapping, please confirm if it looks good? @bharadwaj-aditya
| Oracle Index Type | Spanner Target Mapping | SMT Conversion Logic & Reasoning |
|---|---|---|
| Normal Index | CREATE INDEX |
Migrates natively as a standard Spanner secondary index. |
| Unique Index | CREATE UNIQUE INDEX |
Migrates natively. SMT explicitly enforces the UNIQUE integrity constraint over the Spanner column natively. |
| Composite Index | CREATE INDEX (col1, col2) |
Migrates natively. SMT extracts the exact column sort ordering from all_ind_columns to perfectly recreate the composite hierarchy in Spanner. |
| Bitmap Index | Downgrades to CREATE INDEX |
Migrated as a standard secondary index. Spanner doesn't use low-cardinality bitmap blocks natively, so SMT translates this into a standard secondary index to preserve the query filtering hints safely. |
| Reverse Key Index | Downgrades to CREATE INDEX |
Migrated as a standard secondary index. Spanner prevents RAC hot-block contention dynamically through UUIDs and partition splitting, so the REVERSE bit-hashing logic is stripped. |
| Function-Based Index | Ignored / Filtered | Intentionally dropped. Oracle uses native AST execution functions (e.g., UPPER(col)) that do not securely translate 1:1 on target Spanner instances. |
| ) | ||
|
|
||
| var ( | ||
| projectID string |
There was a problem hiding this comment.
We should retain this test. Unless the functionality is covered in some other test here ?
There was a problem hiding this comment.
I was planning to fix it along with other Integ PR, but I will try fixing this existing integ in this PR and will raise separate PR for new integs.
d08d64a to
80c0150
Compare
80c0150 to
981ef6f
Compare
2 participants
Improve existing oracle support for schema migration in spanner migration tool.