Skip to content

Feature/support cosmos#130

Closed
hmoazam wants to merge 6 commits into
microsoft:mainfrom
hmoazam:feature/support-cosmos
Closed

Feature/support cosmos#130
hmoazam wants to merge 6 commits into
microsoft:mainfrom
hmoazam:feature/support-cosmos

Conversation

@hmoazam

@hmoazam hmoazam commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Implemented support for Azure Cosmos

@hmoazam hmoazam requested a review from wjohnson December 12, 2022 16:14
@wjohnson

Copy link
Copy Markdown
Contributor

@hmoazam would you please also add a unit test to the QNParsers file too?
https://github.com/hmoazam/Purview-ADB-Lineage-Solution-Accelerator/blob/feature/support-kusto/function-app/adb-to-purview/tests/unit-tests/Function.Domain/Helpers/Parser/QnParserTests.cs#L103-L107

[InlineData("OpenLineageNamespaceValue, 
                    "OpenLineageNameValue", 
                    "ExpectedFQN")]

_log.LogError(ex, $"OlMessageConsolodation-JoinEventData: Error {ex.Message} when deleting entity");
}
// Add Inputs to event if not already there (will only be done for DataSourceV2 sources)
if (olEvent.Inputs.Count == 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a case where there may be an input present initially and then more inputs later on?

What about a scenario where ABFSS is used and joined with a Data Source V2 source like Cosmos?

/// from the start and complete events
/// </summary>
private bool isDataSourceV2Event(Event olEvent) {
string[] special_cases = {"azurecosmos://", "iceberg://"}; // todo: make this configurable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed, we should definitely make this part of the configuration instead. Maybe something like openLineageDataSourceV2Prefixes as the parameter and have it separated by semicolon (;)

Similar to the Spark_Entities config. Is there ever a case where "://" would not be part of the prefix?


private bool IsJoinEvent(Event olEvent)
{
string[] special_cases = {"cosmos", "iceberg"};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this get used in here?

/// Performs initial validation of OpenLineage input
/// The tested criteria include:
/// 1. Events have both inputs and outputs
/// a. Except for special cases covered in isDataSourceV2Event

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add why this is necessary? Such as "OpenLineage does not emit a start event or complete event with all of the necessary information and so must be consolidated later on"?

@wjohnson

wjohnson commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

A nitpicky thing :(

function-app/adb-to-purview/src/Function.Domain/Helpers/parser/DatabricksToPurviewParser.cs
function-app/adb-to-purview/src/Function.Domain/Helpers/parser/QnParser.cs
function-app/adb-to-purview/src/Function.Domain/Services/OlConsolodateEnrich.cs
function-app/adb-to-purview/src/Function.Domain/Services/OlToPurviewParsingService.cs
function-app/adb-to-purview/src/Functions/PurviewOut.cs

Do not include any actual code changes, just whitespace. Any chance you'd be willing to undo the whitespace changes just to keep the commit clean and focused on the files necessary to change?

@hmoazam

hmoazam commented Jan 28, 2023

Copy link
Copy Markdown
Contributor Author

Closing in favor of #145

@hmoazam hmoazam closed this Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants