-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: implement support for append_dedup in iceberg dest #48731
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@@ -57,16 +59,26 @@ public InternalRecordWrapper wrapper() { | |||
return wrapper; | |||
} | |||
|
|||
public Record constructIdentifierRecord(Record row) { |
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: does this need to be public?
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
val identifierFields = mutableSetOf<Int>() | ||
val identifierFieldNames = primaryKeys.flatten().toSet() | ||
val icebergTypeConverter = AirbyteTypeToIcebergSchema() | ||
this.properties.entries.forEach { (name, field) -> | ||
val id = generatedSchemaFieldId() | ||
mutableListOf.add( | ||
val isPrimaryKey = identifierFieldNames.contains(name) | ||
val isOptional = !isPrimaryKey && field.nullable |
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.
fyi we tried enforcing PK requiredness at one point and it caused a lot of problems (there's a surprising number of sources that emit null PKs). If iceberg supports null values in a partition key, we should do that instead
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.
It just wont allow you to mark a field as an identifier field if its nullable. Only non-nullable fields can be identifier fields
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.
ugh. I think we Officially Support composite PKs with nullable fields #31926 (see also slack #p0-primay-keys-cannot-be-null)
so that's going to be awkward
No description provided.