(ingestion) bug fix: emit platform instance aspect for dataset in Databricks ingestion

[jinlintt]

Golden file auto generated using the command below.

pytest tests/integration/unity/test_unity_catalog_ingest.py --update-golden-files

[jinlintt]

@hsheth2 @asikowitz
Could you or someone take a look? It is small change and similar to #8585

[asikowitz]

We're aware of this bug, but unfortunately this is going to change all container urns which will not be backwards compatible. I'll talk over with the team the best way to handle this

[jinlintt]

We're aware of this bug, but unfortunately this is going to change all container urns which will not be backwards compatible. I'll talk over with the team the best way to handle this

Thank you @asikowitz , what we really care about is emitting the platform instance aspect for the dataset. I can revert the changes to the container. Let me know.

[asikowitz]

Can you revert the container changes then and regenerate the golden? Please also add a comment on config.platform_instance that it's not consistent with the data platform instance aspect emitted

[jinlintt]

Can you revert the container changes then and regenerate the golden? Please also add a comment on config.platform_instance that it's not consistent with the data platform instance aspect emitted

Done. Regenerated the golden file and no idea why it still has so many churns. @asikowitz

[jinlintt]

@asikowitz and team, could we get this reviewed and merged soon?
We are blocked by this.

[asikowitz]

Hi Jinlin, sorry for the delay. I believe adding this aspect will cause the data platform instance (i.e. workspace name) to appear in the UI as part of the browse path, which we'd generally like to avoid. Just curious, do you want to add this aspect because you're building a feature around it, or just for consistency with the dataset urns.

In any case, for the near future, can you put a flag around creating this aspect, default False? This should also allow you to revert the golden file changes. We may want to produce this in the future, but it'll require some discussion on our side, so I think this is the easiest way to unblock you.

[jinlintt]

Hi Jinlin, sorry for the delay. I believe adding this aspect will cause the data platform instance (i.e. workspace name) to appear in the UI as part of the browse path, which we'd generally like to avoid. Just curious, do you want to add this aspect because you're building a feature around it, or just for consistency with the dataset urns.

In any case, for the near future, can you put a flag around creating this aspect, default False? This should also allow you to revert the golden file changes. We may want to produce this in the future, but it'll require some discussion on our side, so I think this is the easiest way to unblock you.

Hi @asikowitz
Why would you want to hide data platform instance from the browse path? That is always part of the data model. So if it is not a problem for the other data sources such as MySQL, can you explain why this is a problem for Databricks?

I could add a flag to disable this by default, but that seems in consistent with how platform_instance works with other data sources. For the other data sources, setting the platform_instance means the data platform instance aspect will be ingested. But for Databricks, the user has to set one more flag to get it work.

CC @jjoyce0510 @hsheth2

[asikowitz]

For other sources like MySQL, we don't have a default platform instance. Thus, if a user is manually specifying a platform instance, they likely (i) intend to have multiple instances of the same platform and want to distinguish between them and (ii) won't be surprised seeing this platform instance name in the UI. This is different for platform instances that are automatically determined, like here. If a user only has one databricks instance ingested into datahub, they might not want the clutter of seeing their workspace id attached to every databricks dataset.

This is made worse by the fact that we are using a different platform instance name for databricks containers vs datasets, and that we should only be automatically setting platform instance if that value is globally unique. It's not immediately clear to me that our current way of calculating platform_instance_name will be globally unique, e.g. as per https://docs.databricks.com/en/workspace/workspace-details.html, so I don't want to commit further to this value if we have to change this in the future.

We also in general need to clean up how the DataPlatformInstance aspect is generated and how it's represented in the UI. For generation, I am almost positive that databricks is not the only source that's failing to produce DataPlatformInstance aspects; we need to standardize this across the board. However, doing this is blocked by the fact that we don't have a consistent UI for displaying a dataset's "browse path". Some places, we're using the BrowsePaths aspect, another BrowsePathsV2 (which can include platform instance data, but currently will not include platform instance info for databricks as it's determined by config.platform_instance), and another we recursively look up a dataset's container structure and prepend the DataPlatformInstance aspect instance value, if it exists. We likely want to use the BrowsePathsV2 aspect as the source of truth for all UI elements, but until then, we don't want to be making changes by producing more DataPlatformInstance aspects if that's going to produce UI inconsistency, as it will for databricks.

Hopefully that gives you some insight on why, to unblock you, it's simplest to put this behind a flag.

[jinlintt]

@asikowitz thank you for the detailed explanation and it makes sense.
I have added a new flag to guard the ingestion of the DPI aspect and it defaults to false.