-
Notifications
You must be signed in to change notification settings - Fork 97
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
[SNOW-916052] Use python None instead of empty string for tombstone e2e tests #709
Conversation
@@ -566,7 +566,6 @@ public boolean shouldSkipNullValue( | |||
// get valueSchema | |||
Schema valueSchema = record.valueSchema(); | |||
if (valueSchema instanceof SnowflakeJsonSchema) { | |||
// TODO SNOW-916052: will not skip if record.value() == null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valueschema will never be null because snowflake json converter always returns a value schema
@@ -10,8 +10,7 @@ | |||
"snowflake.database.name":"SNOWFLAKE_DATABASE", | |||
"snowflake.schema.name":"SNOWFLAKE_SCHEMA", | |||
"key.converter":"org.apache.kafka.connect.storage.StringConverter", | |||
"value.converter": "org.apache.kafka.connect.json.JsonConverter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the corresponding default tombstone e2e ingestion test uses SnowflakeJsonConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will be the behavior if it is org.apache.kafka.connect.json.JsonConverter? Do we have a test for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonConverter treats empty string as a tombstone record whereas SnowflakeJsonConverter treats it as a normal record.
We don't have snowpipe e2e tests with JsonConverter, but we do have IT tests that test all converters with snowpipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double confirm that we dont have any behavior changes in snowpipe. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment, otherwise LGTM!
@@ -24,7 +24,7 @@ def send(self): | |||
{'numbernumbernumbernumbernumbernumbernumbernumbernumbernumbernumbernumber': str(100)} | |||
).encode('utf-8')) | |||
else: | |||
value.append('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand we need use None for the tombstone tests, but should we have a test to test the empty string behavior instead of updating everything to None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added empty string test and comment to explain the differences between community vs custom converter record behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing and improving test! Left a comment about being cautious in snowpipe code path.
…bstone e2e tests (snowflakedb#709)" This reverts commit 977c07e.
* Revert "[Snyk] Security upgrade org.apache.avro:avro from 1.11.1 to 1.11.3 (snowflakedb#713)" This reverts commit e5fb0d2. * Revert "[SNOW-916052] Use python None instead of empty string for tombstone e2e tests (snowflakedb#709)" This reverts commit 977c07e.
Snowflake converters and community converters convert empty string records different. Snowflake converters treat "" as a valid record, community converters treat "" as a tombstone record. Use python None (null) instead of empty string to accurately test tombstone ingestion
Discussed offline with @sfc-gh-tzhang to use None instead