Merge branch 'master' into gabe--improvingPerfOfElasticGraphQuery

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/WeaklyTypedAspectsResolver.java

@@ -74,12 +74,13 @@ public CompletableFuture<List<RawAspect>> get(DataFetchingEnvironment environmen
 
                     DataMap renderSpec = aspectSpec.getRenderSpec();
 
-                    AspectRenderSpec resultRenderSpec = new AspectRenderSpec();
-
-                    resultRenderSpec.setDisplayType(renderSpec.getString("displayType"));
-                    resultRenderSpec.setDisplayName(renderSpec.getString("displayName"));
-                    resultRenderSpec.setKey(renderSpec.getString("key"));
-                    result.setRenderSpec(resultRenderSpec);
+                    if (renderSpec != null) {
+                        AspectRenderSpec resultRenderSpec = new AspectRenderSpec();
+                        resultRenderSpec.setDisplayType(renderSpec.getString("displayType"));
+                        resultRenderSpec.setDisplayName(renderSpec.getString("displayName"));
+                        resultRenderSpec.setKey(renderSpec.getString("key"));
+                        result.setRenderSpec(resultRenderSpec);
+                    }
 
                     results.add(result);
                 } catch (IOException | RemoteInvocationException | URISyntaxException e) {

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/nocode/DataMigrationStep.java

@@ -17,7 +17,6 @@
 import com.linkedin.metadata.entity.ebean.EbeanAspectV1;
 import com.linkedin.metadata.entity.ebean.EbeanAspectV2;
 import com.linkedin.metadata.models.EntitySpec;
-import com.linkedin.metadata.search.utils.BrowsePathUtils;
 import io.ebean.EbeanServer;
 import io.ebean.PagedList;
 import java.net.URISyntaxException;
@@ -151,7 +150,7 @@ public Function<UpgradeContext, UpgradeStepResult> executable() {
             // Emit a browse path aspect.
             final BrowsePaths browsePaths;
             try {
-              browsePaths = BrowsePathUtils.buildBrowsePath(urn, _entityService.getEntityRegistry());
+              browsePaths = _entityService.buildDefaultBrowsePath(urn);
 
               final AuditStamp browsePathsStamp = new AuditStamp();
               browsePathsStamp.setActor(Urn.createFromString(Constants.SYSTEM_ACTOR));

docker/datahub-gms/env/docker-without-neo4j.env

@@ -43,3 +43,7 @@ ENTITY_SERVICE_ENABLE_RETENTION=true
 # set ELASTICSEARCH_USE_SSL=true and uncomment:
 # ELASTICSEARCH_USERNAME=
 # ELASTICSEARCH_PASSWORD=
+
+# Uncomment to run a one-time upgrade to migrate legacy default browse path format to latest format
+# More details can be found at https://datahubproject.io/docs/advanced/browse-paths-upgrade
+# UPGRADE_DEFAULT_BROWSE_PATHS_ENABLED=true

docker/datahub-gms/env/docker.env

@@ -72,3 +72,7 @@ UI_INGESTION_DEFAULT_CLI_VERSION=0.8.42
 
 # Uncomment to increase concurrency across Kafka consumers
 # KAFKA_LISTENER_CONCURRENCY=2
+
+# Uncomment to run a one-time upgrade to migrate legacy default browse path format to latest format
+# More details can be found at https://datahubproject.io/docs/advanced/browse-paths-upgrade
+# UPGRADE_DEFAULT_BROWSE_PATHS_ENABLED=true

docs-website/sidebars.js

@@ -433,6 +433,7 @@ module.exports = {
           "metadata-ingestion/adding-source",
           "docs/how/add-custom-ingestion-source",
           "docs/how/add-custom-data-platform",
+          "docs/advanced/browse-paths-upgrade",
         ],
       },
     ],

docs/advanced/browse-paths-upgrade.md

@@ -0,0 +1,137 @@
+# Browse Paths Upgrade (August 2022)
+
+## Background
+
+Up to this point, there's been a historical constraint on all entity browse paths. Namely, each browse path has been
+required to end with a path component that represents "simple name" for an entity. For example, a Browse Path for a 
+Snowflake Table called "test_table" may look something like this:
+
+```
+/prod/snowflake/warehouse1/db1/test_table
+```
+
+In the UI, we artificially truncate the final path component when you are browsing the Entity hierarchy, so your browse experience 
+would be: 
+
+`prod` > `snowflake` > `warehouse1`> `db1` > `Click Entity`
+
+As you can see, the final path component `test_table` is effectively ignored. It could have any value, and we would still ignore
+it in the UI. This behavior serves as a workaround to the historical requirement that all browse paths end with a simple name. 
+
+This data constraint stands in opposition the original intention of Browse Paths: to provide a simple mechanism for organizing
+assets into a hierarchical folder structure. For this reason, we've changed the semantics of Browse Paths to better align with the original intention. 
+Going forward, you will not be required to provide a final component detailing the "name". Instead, you will be able to provide a simpler path that
+omits this final component:
+
+```
+/prod/snowflake/warehouse1/db1
+```
+
+and the browse experience from the UI will continue to work as you would expect: 
+
+`prod` > `snowflake` > `warehouse1`> `db1` > `Click Entity`. 
+
+With this change comes a fix to a longstanding bug where multiple browse paths could not be attached to a single URN. Going forward,
+we will support producing multiple browse paths for the same entity, and allow you to traverse via multiple paths. For example
+
+```python
+browse_path = BrowsePathsClass(
+    paths=["/powerbi/my/custom/path", "/my/other/custom/path"]
+)
+return MetadataChangeProposalWrapper(
+    entityType="dataset",
+    changeType="UPSERT",
+    entityUrn="urn:li:dataset:(urn:li:dataPlatform:custom,MyFileName,PROD),
+    aspectName="browsePaths",
+    aspect=browse_path,
+)
+```
+*Using the Python Emitter SDK to produce multiple Browse Paths for the same entity*
+
+We've received multiple bug reports, such as [this issue](https://github.com/datahub-project/datahub/issues/5525), and requests to address these issues with Browse, and thus are deciding
+to do it now before more workarounds are created.  
+
+## What this means for you
+
+Once you upgrade to DataHub `v0.8.45` you will immediately notice that traversing your Browse Path hierarchy will require
+one extra click to find the entity. This is because we are correctly displaying the FULL browse path, including the simple name mentioned above.
+
+There will be 2 ways to upgrade to the new browse path format. Depending on your ingestion sources, you may want to use one or both:
+
+1. Migrate default browse paths to the new format by restarting DataHub
+2. Upgrade your version of the `datahub` CLI to push new browse path format (version `v0.8.45`)
+
+Each step will be discussed in detail below. 
+
+### 1. Migrating default browse paths to the new format
+
+To migrate those Browse Paths that are generated by DataHub by default (when no path is provided), simply restart the `datahub-gms` container / pod with a single
+additional environment variable:
+
+```
+UPGRADE_DEFAULT_BROWSE_PATHS_ENABLED=true
+```
+
+And restart the `datahub-gms` instance. This will cause GMS to perform a boot-time migration of all your existing Browse Paths
+to the new format, removing the unnecessarily name component at the very end.
+
+If the migration is successful, you'll see the following in your GMS logs: 
+
+```
+18:58:17.414 [main] INFO c.l.m.b.s.UpgradeDefaultBrowsePathsStep:60 - Successfully upgraded all browse paths!
+```
+
+After this one-time migration is complete, you should be able to navigate the Browse hierarchy exactly as you did previously. 
+
+> Note that certain ingestion sources actively produce their own Browse Paths, which overrides the default path
+> computed by DataHub. 
+> 
+> In these cases, getting the updated Browse Path will require re-running your ingestion process with the updated
+> version of the connector. This is discussed in more detail in the next section. 
+
+### 2. Upgrading the `datahub` CLI to push new browse paths 
+
+If you are actively ingesting metadata from one or more of following sources
+
+1. Sagemaker
+2. Looker / LookML
+3. Feast
+4. Kafka
+5. Mode
+6. PowerBi
+7. Pulsar
+8. Tableau
+9. Business Glossary
+
+You will need to upgrade the DataHub CLI to >= `v0.8.45` and re-run metadata ingestion. This will generate the new browse path format
+and overwrite the existing paths for entities that were extracted from these sources. 
+
+### If you are producing custom Browse Paths
+
+If you've decided to produce your own custom Browse Paths to organize your assets (e.g. via the Python Emitter SDK), you'll want to change the code to produce those paths
+to truncate the final path component. For example, if you were previously emitting a browse path like this:
+
+```
+"my/custom/browse/path/suffix"
+```
+
+You can simply remove the final "suffix" piece:
+
+```
+"my/custom/browse/path"
+```
+
+Your users will be able to find the entity by traversing through these folders in the UI:
+
+`my` > `custom` > `browse`> `path` > `Click Entity`.
+
+
+> Note that if you are using the Browse Path Transformer you *will* be impacted in the same way. It is recommended that you revisit the
+> paths that you are producing, and update them to the new format. 
+
+## Support
+
+The Acryl team will be on standby to assist you in your migration. Please
+join [#release-0_8_0](https://datahubspace.slack.com/archives/C0244FHMHJQ) channel and reach out to us if you find
+trouble with the upgrade or have feedback on the process. We will work closely to make sure you can continue to operate
+DataHub smoothly.

docs/how/updating-datahub.md

@@ -6,6 +6,9 @@ This file documents any backwards-incompatible changes in DataHub and assists pe
 
 ### Breaking Changes
 
+- Browse Paths have been upgraded to a new format to align more closely with the intention of the feature. 
+  Learn more about the changes, including steps on upgrading, here: https://datahubproject.io/docs/advanced/browse-paths-upgrade
+
 ### Potential Downtime
 
 ### Deprecations

entity-registry/src/main/java/com/linkedin/metadata/models/registry/PatchEntityRegistry.java

@@ -138,14 +138,16 @@ private PatchEntityRegistry(DataSchemaFactory dataSchemaFactory, InputStream con
           entity.getAspects().stream().collect(Collectors.joining()));
       List<AspectSpec> aspectSpecs = new ArrayList<>();
       if (entity.getKeyAspect() != null) {
-        throw new EntityRegistryException(
-            "Patch Entities cannot define entities yet. They can only enhance an existing entity with additional (non-key) aspects");
-        // aspectSpecs.add(getAspectSpec(entity.getKeyAspect(), entitySpecBuilder));
+        AspectSpec keyAspectSpec = buildAspectSpec(entity.getKeyAspect(), entitySpecBuilder);
+        log.info("Adding key aspect {} with spec {}", entity.getKeyAspect(), keyAspectSpec);
+        aspectSpecs.add(keyAspectSpec);
       }
       entity.getAspects().forEach(aspect -> {
-        AspectSpec aspectSpec = buildAspectSpec(aspect, entitySpecBuilder);
-        log.info("Adding aspect {} with spec {}", aspect, aspectSpec);
-        aspectSpecs.add(aspectSpec);
+        if (!aspect.equals(entity.getKeyAspect())) {
+          AspectSpec aspectSpec = buildAspectSpec(aspect, entitySpecBuilder);
+          log.info("Adding aspect {} with spec {}", aspect, aspectSpec);
+          aspectSpecs.add(aspectSpec);
+        }
       });
 
       EntitySpec entitySpec =

entity-registry/src/test/java/com/linkedin/metadata/models/registry/PatchEntityRegistryTest.java

@@ -1,9 +1,10 @@
 package com.linkedin.metadata.models.registry;
 
+import com.linkedin.metadata.models.DataSchemaFactory;
 import com.linkedin.metadata.models.EntitySpec;
 import com.linkedin.metadata.models.EventSpec;
+import java.nio.file.Paths;
 import java.util.Map;
-import org.apache.maven.artifact.versioning.ComparableVersion;
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.*;
@@ -14,7 +15,11 @@ public class PatchEntityRegistryTest {
   @Test
   public void testEntityRegistryLoad() throws Exception, EntityRegistryException {
     PatchEntityRegistry patchEntityRegistry = new PatchEntityRegistry(
-        TestConstants.BASE_DIRECTORY + "/" + TestConstants.TEST_REGISTRY + "/" + TestConstants.TEST_VERSION.toString(),
+        TestConstants.BASE_DIRECTORY 
+        + "/" 
+        + TestConstants.TEST_REGISTRY 
+        + "/" 
+        + TestConstants.TEST_VERSION.toString(),
         TestConstants.TEST_REGISTRY, TestConstants.TEST_VERSION);
 
     Map<String, EntitySpec> entitySpecs = patchEntityRegistry.getEntitySpecs();
@@ -34,14 +39,28 @@ public void testEntityRegistryLoad() throws Exception, EntityRegistryException {
   }
 
   /**
-   * Validate that patch entity registries cannot have key aspects
+   * Validate that patch entity registries can have key aspects
    * @throws Exception
    * @throws EntityRegistryException
    */
   @Test
-  public void testEntityRegistryWithKeyLoad() {
-    assertThrows(EntityRegistryException.class,
-        () -> new PatchEntityRegistry("src/test_plugins/mycompany-full-model/0.0.1", "mycompany-full-model",
-            new ComparableVersion("0.0.1")));
+  public void testEntityRegistryWithKeyLoad() throws Exception, EntityRegistryException {
+    DataSchemaFactory dataSchemaFactory = DataSchemaFactory.withCustomClasspath(
+      Paths.get(TestConstants.BASE_DIRECTORY 
+        + "/" 
+        + TestConstants.TEST_REGISTRY 
+        + "/" 
+        + TestConstants.TEST_VERSION.toString()));
+
+    PatchEntityRegistry patchEntityRegistry = new PatchEntityRegistry(
+      dataSchemaFactory, Paths.get("src/test_plugins/mycompany-full-model/0.0.1/entity-registry.yaml"),
+            TestConstants.TEST_REGISTRY, TestConstants.TEST_VERSION);
+
+    Map<String, EntitySpec> entitySpecs = patchEntityRegistry.getEntitySpecs();
+    assertEquals(entitySpecs.values().size(), 1);
+    EntitySpec newThingSpec = patchEntityRegistry.getEntitySpec("newThing");
+    assertNotNull(newThingSpec);
+    assertNotNull(newThingSpec.getKeyAspectSpec());
+    assertNotNull(newThingSpec.getAspectSpec(TestConstants.TEST_ASPECT_NAME));
   }
 }

entity-registry/src/test_plugins/mycompany-full-model/0.0.1/entity-registry.yaml

@@ -3,6 +3,6 @@
 id: mycompany-full-model
 entities:
   - name: newThing
-    keyAspect: ownership
+    keyAspect: testDataQualityRules
     aspects:
-      - ownership
+      - testDataQualityRules

metadata-ingestion/src/datahub/ingestion/api/report.py

@@ -2,7 +2,7 @@
 import pprint
 import sys
 from dataclasses import dataclass
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
 from enum import Enum
 from typing import Any, Dict
 
@@ -24,19 +24,26 @@ def to_str(some_val: Any) -> str:
         elif isinstance(some_val, timedelta):
             return humanfriendly.format_timespan(some_val)
         elif isinstance(some_val, datetime):
-            now = datetime.now()
-            diff = now - some_val
-            if abs(diff) < timedelta(seconds=1):
-                # the timestamps are close enough that printing a duration isn't useful
-                return f"{some_val} (now)."
-            elif diff > timedelta(seconds=0):
-                # timestamp is in the past
-                return f"{some_val} ({humanfriendly.format_timespan(diff)} ago)."
-            else:
-                # timestamp is in the future
-                return (
-                    f"{some_val} (in {humanfriendly.format_timespan(some_val - now)})."
+            try:
+                # check if we have a tz_aware object or not (https://stackoverflow.com/questions/5802108/how-to-check-if-a-datetime-object-is-localized-with-pytz)
+                tz_aware = (
+                    some_val.tzinfo is not None
+                    and some_val.tzinfo.utcoffset(some_val) is not None
                 )
+                now = datetime.now(timezone.utc) if tz_aware else datetime.now()
+                diff = now - some_val
+                if abs(diff) < timedelta(seconds=1):
+                    # the timestamps are close enough that printing a duration isn't useful
+                    return f"{some_val} (now)."
+                elif diff > timedelta(seconds=0):
+                    # timestamp is in the past
+                    return f"{some_val} ({humanfriendly.format_timespan(diff)} ago)."
+                else:
+                    # timestamp is in the future
+                    return f"{some_val} (in {humanfriendly.format_timespan(some_val - now)})."
+            except Exception:
+                # we don't want to fail reporting because we were unable to pretty print a timestamp
+                return str(datetime)
         else:
             return str(some_val)
 

metadata-ingestion/src/datahub/ingestion/source_config/bigquery.py

@@ -35,4 +35,4 @@ def sharded_table_pattern_is_a_valid_regexp(cls, v):
             raise ConfigurationError(
                 f"sharded_table_pattern configuration pattern is invalid. The exception was: {e}"
             )
-        return
+        return v