-
Notifications
You must be signed in to change notification settings - Fork 4k
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
convert destination-bigquery to kotlin CDK #36899
convert destination-bigquery to kotlin CDK #36899
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @stephane-airbyte and the rest of your teammates on Graphite |
.../main/kotlin/io/airbyte/integrations/base/destination/typing_deduping/DefaultTyperDeduper.kt
Outdated
Show resolved
Hide resolved
@@ -48,7 +48,7 @@ public void flush(final StreamDescriptor decs, final Stream<PartialAirbyteMessag | |||
|
|||
stream.forEach(record -> { | |||
try { | |||
writer.accept(record.getSerialized(), record.getRecord().getEmittedAt()); | |||
writer.accept(record.getSerialized(), Jsons.serialize(record.getRecord().getMeta()), record.getRecord().getEmittedAt()); |
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.
@gisripa I think this is what I need to do, but I'd like you to confirm 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.
looks correct to me. One thing to verify is if BigQuery is ok adding extra stuff in the CSV file, we had to pass a flag in snowflake to ignore this meta
information until we add the migration code to populate meta
in raw table.
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 think that my addition of V2_WITH_META means that this value will be ignored when writing the CSV
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.
yeah looks like it. seems ok, when we introduce meta in BQ this will work.
1467569
to
a02d678
Compare
4fea7a9
to
c2074de
Compare
airbyte-ci/connectors/pipelines/tests/test_format/non_formatted_code/java.java
Outdated
Show resolved
Hide resolved
0d28feb
to
3598443
Compare
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.
not sure what's up with most of the test failures (... they're claiming that we failed to write the data correctly, which sounds sketchy)
but at least this one I think is just an unrelated breaking change in the cdk - lmk if you want a hand with it, but you should be able to mostly copy these files from e.g. redshift's test resources
BigQuerySqlGeneratorIntegrationTest > testV1V2migration() FAILED
org.gradle.internal.exceptions.DefaultMultiCauseException: Multiple Failures (2 failures)
java.lang.IllegalArgumentException: resource sqlgenerator/alltypes_v1v2_expectedrecords_raw.jsonl not found.
java.lang.IllegalArgumentException: resource sqlgenerator/alltypes_v1v2_expectedrecords_final.jsonl not found.
* | ||
* The @JvmSuppressWildcards is here so that the 2nd parameter of accept stays a java | ||
* Map<StreamDescriptor, StreamSyncSummary> | ||
* ``` |
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.
nit: this formatting looks off? (also TIL JvmSuppressWildcards)
The failures seems pretty much what we encountered in Snowflake where adding additional data in CSV ( |
https://cloud.google.com/bigquery/docs/loading-data-cloud-storage-csv |
9a617ca
to
0b9fa0f
Compare
b456f57
to
41db851
Compare
/publish-java-cdk
|
2584ef3
to
1a27dfe
Compare
defaultNamespace = defaultNamespace, | ||
flushFailure = FlushFailure(), | ||
workerPool = workerPool, | ||
) |
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 aggressively removed the overloaded constructors hoping the 2 usages were already in Kotlin with defaults injected. Here it is yet another Java usage 🤦♂️
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.
yep, I noticed your removal 😠 😏
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.
one nit, otherwise lgtm
.../io/airbyte/integrations/base/destination/typing_deduping/BaseSqlGeneratorIntegrationTest.kt
Show resolved
Hide resolved
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.
lgtm.
nit: any chance we can avoid that overloaded constructor in AsyncStreamConsumer, even if it means copy-pasting the required ones from StagingConsumerFactory into BQ land ?
1a27dfe
to
226ee35
Compare
/publish-java-cdk
|
226ee35
to
d60c192
Compare
/publish-java-cdk force=true
|
d60c192
to
5139ad8
Compare
5139ad8
to
b5ffe32
Compare
b5ffe32
to
7a68dfd
Compare
Merge activity
|
No description provided.