Fix incorrect schema change detection in spanner cdc to bq template #2183
Conversation
This file contains 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2183 +/- ##
=========================================
Coverage 46.97% 46.97%
- Complexity 4049 4050 +1
=========================================
Files 876 876
Lines 52193 52193
Branches 5502 5502
=========================================
+ Hits 24517 24519 +2
+ Misses 25920 25919 -1
+ Partials 1756 1755 -1
|
nancyxu825
reviewed
Feb 14, 2025
| @@ -42,8 +42,8 @@ public static boolean detectDiffColumnInMod( | |||
| mod.getNewValuesJson() == "" | |||
| ? new JSONObject("{}").keySet() | |||
| : new JSONObject(mod.getNewValuesJson()).keySet(); | |||
| // At this mod's spannerCommitTimestamp, one column is added/dropped. | |||
There was a problem hiding this comment.
What if the user does end up dropping the column, but we don't update the stored schema?
Will we still do a read to Spanner for the dropped column? Will that cause NOT_FOUND errors?
There was a problem hiding this comment.
You are absolutely right. Seems querying info schema columns is unavoidable even if there is no schema changes but this mod did not modify all tracking columns. This feels pretty inefficient.
What do you think about making this configurable? ie: Disable support schema update handling through a config. If disabled, we don't perform these info schema queries.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We should only query information schema if the recorded tracked columns is less than the # of columns from a mod for
NEW_VALUESandOLD_AND_NEW_VALUES. For these two val capture types, it is expected to have less columns in new values json compare to all tracking columns and this doesn't necessarily means there are schema updates.Basically
detectDiffColumnInModshould return false ifkeySetOfNewValuesJsonObjectis a subset ofspannerTable.getNonPkColumns().Before the fix the information schema columns query would trigger more often than needed, whenever there are less columns in new values json compare to all tracking columns for
NEW_VALUESandOLD_AND_NEW_VALUES. This is more likely to occur if user has just add a new column and only try to only update that new column(where mod will only contain the new column).