[Spark][Kernel] Pass table identifier in Coordinated Commits #3608

prakharjain09 · 2024-08-27T05:32:02Z

Which Delta project/connector is this regarding?

Description

This PR adds TableIdentifier in the Coordinated Commit Interface.

There will be a separate change to pass the tableName reliably to the Commit Coordinator in delta-spark.

How was this patch tested?

Existing UTs

Does this PR introduce any user-facing changes?

No

scovich · 2024-08-27T12:42:35Z

Missing some newly added files by chance?

[error] /home/runner/work/delta/delta/storage/src/main/java/io/delta/storage/commit/CommitCoordinatorClient.java:73:1: cannot find symbol
[error]   symbol:   class TableIdentifier
[error]   location: interface io.delta.storage.commit.CommitCoordinatorClient
[error] TableIdentifier
[error] /home/runner/work/delta/delta/storage/src/main/java/io/delta/storage/commit/CommitCoordinatorClient.java:97:1: cannot find symbol
[error]   symbol:   class TableDescriptor
[error]   location: interface io.delta.storage.commit.CommitCoordinatorClient
[error] TableDescriptor

prakharjain09 · 2024-08-27T21:50:42Z

Missing some newly added files by chance?

[error] /home/runner/work/delta/delta/storage/src/main/java/io/delta/storage/commit/CommitCoordinatorClient.java:73:1: cannot find symbol
[error] symbol: class TableIdentifier
[error] location: interface io.delta.storage.commit.CommitCoordinatorClient
[error] TableIdentifier
[error] /home/runner/work/delta/delta/storage/src/main/java/io/delta/storage/commit/CommitCoordinatorClient.java:97:1: cannot find symbol
[error] symbol: class TableDescriptor
[error] location: interface io.delta.storage.commit.CommitCoordinatorClient
[error] TableDescriptor

Yes - fixed it.

scovich

This PR seems to mix two things:

Introducing the TableDescriptor and plumbing it through all the various method signatures and call sites. Important, but very noisy.
Introducing the TableIdentifier and barely starting to plumb it in a couple of spots (which just pass a hard-wired empty option).

Can we move the second part to the PR that actually plumbs through table identifiers in a meaningful way? Then this PR would be a lot easier to review, and probably that later PR as well (plus meaningful testing becomes possible there).

prakharjain09 · 2024-08-29T17:53:01Z

Can we move the second part to the PR that actually plumbs through table identifiers in a meaningful way?

The changes to plumb TableIdentifier is a superset of the changes to plumb TableDescriptor. This PR focuses on the TableDescriptor part and together with it also takes care of plumbing TableIdentifier in those areas. Have left specific TODOs to fix the call-sites and pass a non-empty TableIdentifier which will be taken in next set of PRs.

vkorukanti

Kernel changes LGTM

...lts/src/main/java/io/delta/kernel/defaults/engine/DefaultCommitCoordinatorClientHandler.java

scovich

LGTM. Just double check some files that may import java Optional unnecessarily?

scovich · 2024-08-29T20:29:56Z

...k/src/main/scala/org/apache/spark/sql/delta/coordinatedcommits/CoordinatedCommitsUtils.scala

+        catalystTableIdentifier.catalog.toSeq ++
+          catalystTableIdentifier.database.toSeq
+      new TableIdentifier(namespace.toArray, catalystTableIdentifier.table)
+    }.map(Optional.of[TableIdentifier]).getOrElse(Optional.empty[TableIdentifier])


There isn't a(ny easy) way to directly convert a scala Option into a java Optional?
https://www.javadoc.io/doc/org.scala-lang.modules/scala-java8-compat_2.11/0.6.0/scala/compat/java8/OptionConverters.html
https://www.scala-lang.org/api/2.13.6/scala/jdk/OptionConverters$.html

(maybe it's a scala-2.13 thing we don't have a compat shim for yet?)

Confirmed we currently lack that particular shim... https://github.com/delta-io/delta/blob/master/spark/src/main/scala/org/apache/spark/sql/util/ScalaExtensions.scala#L22

Maybe we should consider adding it as a follow-up item, since coordinated commit code has a lot of scala/java interop?

scovich · 2024-08-29T20:33:59Z

spark/src/test/scala/org/apache/spark/sql/delta/OptimisticTransactionSuite.scala

@@ -18,6 +18,7 @@ package org.apache.spark.sql.delta

 // scalastyle:off import.ordering.noEmptyLine
 import java.nio.file.FileAlreadyExistsException
+import java.util.Optional


Unused import?

scovich · 2024-08-29T23:20:16Z

spark/src/test/scala/org/apache/spark/sql/delta/test/DeltaTestImplicits.scala

@@ -91,6 +93,40 @@ object DeltaTestImplicits {
    }
  }

+  /** Helper class for working with [[TableCommitCoordinatorClient]] */


These helpers avoid having to retrofit zillions of call sites in path-based unit tests?

Yes - these are very useful.

prakharjain09 force-pushed the prakharjain09/cc-api-tableId-temp branch from 52447f6 to a33c191 Compare August 27, 2024 05:32

prakharjain09 force-pushed the prakharjain09/cc-api-tableId-temp branch 2 times, most recently from 48b79ff to 668920b Compare August 27, 2024 21:49

prakharjain09 requested a review from scovich August 27, 2024 21:49

scovich reviewed Aug 27, 2024

View reviewed changes

prakharjain09 force-pushed the prakharjain09/cc-api-tableId-temp branch from 668920b to d94a825 Compare August 29, 2024 17:15

vkorukanti added kernel kernel-api-change labels Aug 29, 2024

prakharjain09 changed the title ~~[WIP][Spark][Kernel] Pass table identifier in Coordinated Commits~~ [Spark][Kernel] Pass table identifier in Coordinated Commits Aug 29, 2024

prakharjain09 added 2 commits August 29, 2024 10:49

Changes for TableIdentifier

4757bcd

Kernel specific change

9775eac

prakharjain09 force-pushed the prakharjain09/cc-api-tableId-temp branch from d94a825 to 9775eac Compare August 29, 2024 17:50

prakharjain09 requested review from scovich and vkorukanti August 29, 2024 18:05

vkorukanti approved these changes Aug 29, 2024

View reviewed changes

...lts/src/main/java/io/delta/kernel/defaults/engine/DefaultCommitCoordinatorClientHandler.java Outdated Show resolved Hide resolved

fix

79d5bde

scovich approved these changes Aug 29, 2024

View reviewed changes

fix

bf42d08

vkorukanti merged commit a745000 into delta-io:master Aug 30, 2024
14 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spark][Kernel] Pass table identifier in Coordinated Commits #3608

[Spark][Kernel] Pass table identifier in Coordinated Commits #3608

prakharjain09 commented Aug 27, 2024 •

edited

Loading

scovich commented Aug 27, 2024

prakharjain09 commented Aug 27, 2024

scovich left a comment •

edited

Loading

prakharjain09 commented Aug 29, 2024

vkorukanti left a comment

scovich left a comment

scovich Aug 29, 2024

scovich Aug 29, 2024

scovich Aug 29, 2024

scovich Aug 29, 2024

prakharjain09 Aug 30, 2024

[Spark][Kernel] Pass table identifier in Coordinated Commits #3608

[Spark][Kernel] Pass table identifier in Coordinated Commits #3608

Conversation

prakharjain09 commented Aug 27, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

scovich commented Aug 27, 2024

prakharjain09 commented Aug 27, 2024

scovich left a comment • edited Loading

Choose a reason for hiding this comment

prakharjain09 commented Aug 29, 2024

vkorukanti left a comment

Choose a reason for hiding this comment

scovich left a comment

Choose a reason for hiding this comment

scovich Aug 29, 2024

Choose a reason for hiding this comment

scovich Aug 29, 2024

Choose a reason for hiding this comment

scovich Aug 29, 2024

Choose a reason for hiding this comment

scovich Aug 29, 2024

Choose a reason for hiding this comment

prakharjain09 Aug 30, 2024

Choose a reason for hiding this comment

prakharjain09 commented Aug 27, 2024 •

edited

Loading

scovich left a comment •

edited

Loading