Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: MAP value converter for data via debezium #2156

Merged
merged 32 commits into from
Jan 20, 2025
Merged

Conversation

priyanshi-yb
Copy link
Contributor

@priyanshi-yb priyanshi-yb commented Jan 6, 2025

Describe the changes in this pull request

  1. Fix hstore datatype value converter (MAP) to convert values properly from debezium in debezium exporter.
  2. Report the store datatype in Unsupported datatype for live migration with fall-forward/fall-back as gRPC connector doesn't support it yet.
    https://yugabyte.atlassian.net/browse/DB-14753

Describe if there are any user-facing changes

No

How was this pull request tested?

end-to-end tests

Does your PR have changes that can cause upgrade issues?

Component Breaking changes?
MetaDB No
Name registry json No
Data File Descriptor Json No
Export Snapshot Status Json No
Import Data State No
Export Status Json No
Data .sql files of tables No
Export and import data queue No
Schema Dump No
AssessmentDB No
Sizing DB No
Migration Assessment Report Json No
Callhome Json No
YugabyteD Tables No
TargetDB Metadata Tables No

@priyanshi-yb priyanshi-yb marked this pull request as ready for review January 8, 2025 07:37
yb-voyager/src/tgtdb/suites/yugabytedbSuite.go Outdated Show resolved Hide resolved
yb-voyager/src/tgtdb/suites/yugabytedbSuite.go Outdated Show resolved Hide resolved
yb-voyager/src/tgtdb/suites/yugabytedbSuite.go Outdated Show resolved Hide resolved
yb-voyager/src/tgtdb/suites/yugabytedbSuite.go Outdated Show resolved Hide resolved
yb-voyager/src/tgtdb/suites/yugabytedbSuite.go Outdated Show resolved Hide resolved
…um.data.Json converter to convert it properly for complex cases as well
case MAP:
StringBuilder mapString = new StringBuilder();
for (Map.Entry<String, String> entry : ((HashMap<String, String>) fieldValue).entrySet()) {
String key = entry.getKey();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add debug logs before and after our transformation for the key and value.

@@ -136,6 +136,14 @@ main() {
fi
fi

if [ "${TEST_DIR}" = "${TESTS_DIR}/pg/datatypes" ]; then
cat ${EXPORT_DIR}/data/hstore_example_data.sql
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just for debugging? remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -26,6 +26,7 @@ EXPECTED_ROW_COUNT = {
'datetime_type2': 2,
'null_and_default' :2,
'decimal_types': 3,
'hstore_example': 13,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also add data validations (for each row), for source-target, source-source-replica, target-source-replica(ignore if this is not supported).

You can use get_distinct_values_of_column_of_table

transformedMapValue = transformedMapValue + fmt.Sprintf("\"%s\"=>\"%s\",", key, value)
}
return fmt.Sprintf("'%s'", transformedMapValue[:len(transformedMapValue)-1]), nil //remove last comma and add quotes
"MAP": func(columnValue string, formatIfRequired bool, dbzmSchema *schemareg.ColumnSchema) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • what about postgressuite? Do we re-use yugabytedbSuite?
  • we have some implementation for "MAP" in oracleSuite as well. I don't imagine that is useful, since we won't have MAP coming from oracle source at all, so remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about postgressuite? Do we re-use yugabytedbSuite?

yes we use the same for both

we have some implementation for "MAP" in oracleSuite as well. I don't imagine that is useful, since we won't have MAP coming from oracle source at all, so remove it?

yeah ok

@@ -130,6 +130,14 @@ def migration_completed_checks(tgt):
expected_distinct_values = EXPECTED_DISNTICT_VALUES['v5']
tgt.assert_distinct_values_of_col("datatypes2", "v5", "public",
None, expected_distinct_values = expected_distinct_values)

print("hstore_example:")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would run for all of target, source-replica, source(in case of fb), right?
how is it passing for fb, if YB cdc connector does not support it?

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it runs on all target, source, and source-replica.
For the fall-forward/fall-back workflows we still run with the hstore_example table and snapshot data is still present in all the DBs but the CDC events are not added for the target_delta i.e. after cutover to target. So only CDC events from target for hstore are not getting tested.

Copy link
Collaborator

@makalaaneesh makalaaneesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@priyanshi-yb priyanshi-yb merged commit ae2a72f into main Jan 20, 2025
67 checks passed
@priyanshi-yb priyanshi-yb deleted the priyanshi/fix-hstore branch January 20, 2025 13:40
Comment on lines +99 to +106
mapString.append("\"");
mapString.append(key);
mapString.append("\"");
mapString.append(" => ");
mapString.append("\"");
mapString.append(val);
mapString.append("\"");
mapString.append(",");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@priyanshi-yb, do we have something like fmt.Sprintf() in Java to use here?

Copy link
Collaborator

@makalaaneesh makalaaneesh Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 that would be a better. No idea why I wrote it like this initially 😀

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is this String.format() which looks similar to fmt.Sprintf() can fix that in a follow up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants