-
Notifications
You must be signed in to change notification settings - Fork 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(ingest/snowflake): allow option for incremental properties #12080
feat(ingest/snowflake): allow option for incremental properties #12080
Conversation
return MetadataWorkUnit(id=MetadataWorkUnit.generate_workunit_id(mcp), mcp_raw=mcp) | ||
|
||
|
||
def auto_incremental_properties( |
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.
This seems to convert the entire aspect to a PATCH. For now, can we just patch the custom properties, to limit surface area?
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 don't think it would work as expected if we emit rest of aspect as a whole and only custom properties as patch. Emitting the whole aspect will overwrite any parallel edits made to datasetProperties.
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.
Is there any plan or intention to make this generic to other entities beyond Dataset?
Current implementation is specific for DatasetProperties
aspect, however the namings (ingremental_properties_helper.py
, IncrementalPropertiesConfigMixin
, ...) suggest this could apply for the properties of any entity (Container, ...).
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.
That's a very good question and I did consider it myself.
I was hoping, we can reuse this for rest of entity types as well in future.
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.
If keeping generic is the plan, we may provide a generic flag description too, by mentioning entities instead of dataset.
Also, we may add some conditional logic when fetching the DatasetPropertiesClass
instead of assuming it's a dataset entity; for the moment, we can skip processing other entity types. WDYT?
properties_aspect = wu.get_aspect_of_type(DatasetPropertiesClass)
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.
DatasetPropertiesClass is only present for entity dataset. For other entities, it would be absent so would return None. So its safe.
I would update description once this config and workunit processor is adopted for other entities. Also planning a quick followup with addressing TODOs in this PR. Will make changes in there.
355a7e6
into
datahub-project:master
Checklist