Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore() Set a default lineage filtering end time on backend when a start time is present #10925

Merged
merged 4 commits into from
Jul 18, 2024
Merged

chore() Set a default lineage filtering end time on backend when a start time is present #10925

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5f5a82
Adding some new things
Jul 16, 2024
8e8b57f
oss changes
Jul 16, 2024
4edc522
Adding this
Jul 17, 2024
8c779b4
Adding this
Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,4 +255,19 @@ public static Filter viewFilter(
Filter result = SearchUtils.combineFilters(null, viewInfo.getDefinition().getFilter());
return result;
}

/**
* Simply resolves the end time filter for the search across lineage query. If the start time is
* provided, but end time is not provided, we will default to the current time.
*/
public static Long getLineageEndTimeMillis(
@Nullable Long startTimeMillis, @Nullable Long endTimeMillis) {
if (endTimeMillis != null) {
return endTimeMillis;
}
if (startTimeMillis != null) {
return System.currentTimeMillis();
}
return null;
}
Comment on lines +258 to +272
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-Implemented Method for Defaulting endTimeMillis

The new method getLineageEndTimeMillis is implemented correctly. It provides a robust way to handle default end time values based on the presence of start time. This method is central to the PR's functionality and is well-integrated across the necessary files.

Would you like me to help in writing unit tests for this method to cover various edge cases?

}
6 changes: 5 additions & 1 deletion ...rc/main/java/com/linkedin/datahub/graphql/resolvers/load/EntityLineageResultResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.linkedin.datahub.graphql.generated.LineageInput;
import com.linkedin.datahub.graphql.generated.LineageRelationship;
import com.linkedin.datahub.graphql.generated.Restricted;
import com.linkedin.datahub.graphql.resolvers.ResolverUtils;
import com.linkedin.datahub.graphql.types.common.mappers.UrnToEntityMapper;
import com.linkedin.metadata.graph.SiblingGraphService;
import graphql.schema.DataFetcher;
Expand Down Expand Up @@ -63,7 +64,10 @@ public CompletableFuture<EntityLineageResult> get(DataFetchingEnvironment enviro
@Nullable final Integer count = input.getCount(); // Optional!
@Nullable final Boolean separateSiblings = input.getSeparateSiblings(); // Optional!
@Nullable final Long startTimeMillis = input.getStartTimeMillis(); // Optional!
@Nullable final Long endTimeMillis = input.getEndTimeMillis(); // Optional!
@Nullable
final Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(
input.getStartTimeMillis(), input.getEndTimeMillis()); // Optional!

com.linkedin.metadata.graph.LineageDirection resolvedDirection =
com.linkedin.metadata.graph.LineageDirection.valueOf(lineageDirection.toString());
Expand Down
3 changes: 2 additions & 1 deletion .../main/java/com/linkedin/datahub/graphql/resolvers/search/ScrollAcrossLineageResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ public CompletableFuture<ScrollAcrossLineageResults> get(DataFetchingEnvironment
@Nullable
Long startTimeMillis = input.getStartTimeMillis() == null ? null : input.getStartTimeMillis();
@Nullable
Long endTimeMillis = input.getEndTimeMillis() == null ? null : input.getEndTimeMillis();
Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(input.getStartTimeMillis(), input.getEndTimeMillis());

final LineageFlags lineageFlags = LineageFlagsInputMapper.map(context, input.getLineageFlags());
if (lineageFlags.getStartTimeMillis() == null && startTimeMillis != null) {
Expand Down
3 changes: 2 additions & 1 deletion .../main/java/com/linkedin/datahub/graphql/resolvers/search/SearchAcrossLineageResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ public CompletableFuture<SearchAcrossLineageResults> get(DataFetchingEnvironment
@Nullable
Long startTimeMillis = input.getStartTimeMillis() == null ? null : input.getStartTimeMillis();
@Nullable
Long endTimeMillis = input.getEndTimeMillis() == null ? null : input.getEndTimeMillis();
Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(input.getStartTimeMillis(), input.getEndTimeMillis());

final LineageFlags lineageFlags = LineageFlagsInputMapper.map(context, input.getLineageFlags());
if (lineageFlags.getStartTimeMillis() == null && startTimeMillis != null) {
Expand Down
11 changes: 8 additions & 3 deletions .../main/java/com/linkedin/datahub/graphql/types/common/mappers/LineageFlagsInputMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.linkedin.datahub.graphql.QueryContext;
import com.linkedin.datahub.graphql.generated.EntityTypeToPlatforms;
import com.linkedin.datahub.graphql.generated.LineageFlags;
import com.linkedin.datahub.graphql.resolvers.ResolverUtils;
import com.linkedin.datahub.graphql.types.entitytype.EntityTypeMapper;
import com.linkedin.datahub.graphql.types.mappers.ModelMapper;
import java.util.Collections;
Expand Down Expand Up @@ -42,12 +43,16 @@ public com.linkedin.metadata.query.LineageFlags apply(
if (lineageFlags.getIgnoreAsHops() != null) {
result.setIgnoreAsHops(mapIgnoreAsHops(lineageFlags.getIgnoreAsHops()));
}
if (lineageFlags.getEndTimeMillis() != null) {
result.setEndTimeMillis(lineageFlags.getEndTimeMillis());
}
if (lineageFlags.getStartTimeMillis() != null) {
result.setStartTimeMillis(lineageFlags.getStartTimeMillis());
}
// Default to "now" if no end time is provided, but start time is provided.
Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(
lineageFlags.getStartTimeMillis(), lineageFlags.getEndTimeMillis());
if (endTimeMillis != null) {
result.setEndTimeMillis(lineageFlags.getEndTimeMillis());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Bug in Setting endTimeMillis

The method correctly calculates endTimeMillis using ResolverUtils.getLineageEndTimeMillis, but it incorrectly sets this value in the result object. It should set the calculated endTimeMillis instead of the one possibly null from lineageFlags.

- result.setEndTimeMillis(lineageFlags.getEndTimeMillis());
+ result.setEndTimeMillis(endTimeMillis);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Default to "now" if no end time is provided, but start time is provided.
Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(
lineageFlags.getStartTimeMillis(), lineageFlags.getEndTimeMillis());
if (endTimeMillis != null) {
result.setEndTimeMillis(lineageFlags.getEndTimeMillis());
}
// Default to "now" if no end time is provided, but start time is provided.
Long endTimeMillis =
ResolverUtils.getLineageEndTimeMillis(
lineageFlags.getStartTimeMillis(), lineageFlags.getEndTimeMillis());
if (endTimeMillis != null) {
result.setEndTimeMillis(endTimeMillis);
}

if (lineageFlags.getEntitiesExploredPerHopLimit() != null) {
result.setEntitiesExploredPerHopLimit(lineageFlags.getEntitiesExploredPerHopLimit());
}
Expand Down
Loading