[DRAFT] Add changes to populate the data source metadata details #25127

evanvdia · 2025-05-15T15:00:46Z

Description

To achieve combined lineage tracking, add changes to populate the data source metadata details.

Motivation and Context

Issue: #25123

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

evanvdia · 2025-05-16T10:48:19Z

@imjalpreet

imjalpreet

@evanvdia Thanks for the PR, I did a first pass and added a few suggestions.

imjalpreet · 2025-05-20T12:38:34Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcInputInfo.java

@evanvdia I don't see this class being referenced anywhere. Is the PR missing some changes?

@imjalpreet I have added the changes.

imjalpreet · 2025-05-20T12:44:44Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcOutputMetaData.java

+
+import static java.util.Objects.requireNonNull;
+
+public class JdbcOutputMetaData


nit: please rename to JdbcOutputMetadata

imjalpreet · 2025-05-20T12:48:46Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcOutputMetaData.java

+    }
+
+    @JsonProperty
+    public String getInfo()


nit: please add @Override annotation as well.

imjalpreet · 2025-05-20T12:52:54Z

presto-hive-common/src/main/java/com/facebook/presto/hive/HiveConnectorOutputMetadata.java

+import java.util.List;
+import java.util.Objects;
+
+public class HiveConnectorOutputMetadata


nit: please rename this class as HiveOutputInfo

imjalpreet · 2025-05-20T12:53:45Z

presto-hive-common/src/main/java/com/facebook/presto/hive/HiveConnectorOutputMetadata.java

+            @JsonProperty("partitionNames") List<String> partitionNames,
+            @JsonProperty("tableLocation") String tableLocation)
+    {
+        this.partitionNames = partitionNames;


please add null checks

imjalpreet · 2025-05-20T12:57:28Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

                partitionUpdates.stream()
                        .map(PartitionUpdate::getName)
-                        .collect(toList())));
+                        .collect(toList()), writeInfo.getTargetPath().getName())));


Why are we using writeInfo.getTargetPath().toString() in one case and writeInfo.getTargetPath().getName() in the other?

imjalpreet · 2025-05-20T12:58:47Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveWrittenPartitions.java


 import static java.util.Objects.requireNonNull;

 public class HiveWrittenPartitions
        implements ConnectorOutputMetadata
 {
-    private final List<String> partitionNames;
+    private final HiveConnectorOutputMetadata connectorOutputInfo;


please rename the object as hiveOutputInfo

imjalpreet · 2025-05-20T12:59:13Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveWrittenPartitions.java

    {
-        this.partitionNames = ImmutableList.copyOf(requireNonNull(partitionNames, "partitionNames is null"));
+        this.connectorOutputInfo = requireNonNull(connectorOutputInfo, "connectorOutputInfo is null");
    }

    @JsonProperty


Add an @Override annotation here as well

imjalpreet · 2025-05-20T13:08:17Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

+        IcebergTableLayoutHandle icebergTableHandle = (IcebergTableLayoutHandle) tableHandle;
+        return Optional.of(new IcebergInputInfo(
+                icebergTableHandle.getTable().getIcebergTableName().getSnapshotId(),
+                icebergTableHandle.getTable().getOutputPath().get()));


nit: please add null checks

imjalpreet · 2025-05-20T13:13:21Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java

@@ -570,9 +571,9 @@ private Optional<ConnectorOutputMetadata> finishInsert(ConnectorSession session,
            throw new PrestoException(ICEBERG_COMMIT_ERROR, "Failed to commit Iceberg update to table: " + writableTableHandle.getTableName(), e);
        }

-        return Optional.of(new HiveWrittenPartitions(commitTasks.stream()
+        return Optional.of(new HiveWrittenPartitions(new HiveConnectorOutputMetadata(commitTasks.stream()


I noticed we are not using IcebergWrittenPartitions here, and the class is unused. We can do a small refactor. Let's remove IcebergWrittenPartitions and also move HiveWrittenPartitions from presto-hive to presto-hive-common.

evanvdia requested review from hantangwangd, ZacBlanco and a team as code owners May 15, 2025 15:00

evanvdia requested a review from jaystarshot May 15, 2025 15:00

prestodb-ci added the from:IBM PR from IBM label May 15, 2025

prestodb-ci requested review from a team, ScrapCodes and BryanCutler and removed request for a team May 15, 2025 15:00

evanvdia force-pushed the datasource_details_at_event branch from 2bf5773 to 9f9bc37 Compare May 15, 2025 15:02

evanvdia marked this pull request as draft May 15, 2025 15:53

evanvdia force-pushed the datasource_details_at_event branch from 9f9bc37 to cd7ec48 Compare May 16, 2025 09:14

imjalpreet requested changes May 20, 2025

View reviewed changes

imjalpreet reviewed May 20, 2025

View reviewed changes

evanvdia force-pushed the datasource_details_at_event branch 2 times, most recently from facd048 to 5aaf7e9 Compare May 21, 2025 09:20

evanvdia requested a review from imjalpreet May 21, 2025 16:59

Add changes to populate the datasource metadata details

a1a4003

evanvdia force-pushed the datasource_details_at_event branch from 5aaf7e9 to a1a4003 Compare May 22, 2025 03:43


		import static java.util.Objects.requireNonNull;

		public class JdbcOutputMetaData

[DRAFT] Add changes to populate the data source metadata details #25127

Are you sure you want to change the base?

[DRAFT] Add changes to populate the data source metadata details #25127

Uh oh!

Conversation

evanvdia commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

evanvdia commented May 16, 2025

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanvdia commented May 15, 2025 •

edited

Loading