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.

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

[Spark] Replaces startTransaction(Snapshot) calls with catalogTable overload #2126

Conversation

LukasRupprecht
Copy link
Contributor

Which Delta project/connector is this regarding?

  • Spark
  • Standalone
  • Flink
  • Kernel
  • Other (fill in here)

Description

This PR replaces all calls to DeltaLog.startTransaction(Snapshot) with calls to DeltaLog.startTransaction(Option[CatalogTable], Snapshot). This PR is part of #2105 and a follow-up to #2125. It makes sure that transactions have a valid catalogTable attached to them so Uniform can correctly update the table in the catalog.

How was this patch tested?

This is a small refactoring change so existing test coverage is sufficient.

Does this PR introduce any user-facing changes?

No

@LukasRupprecht LukasRupprecht changed the title [Spark] Replaces startTransaction(Snapshot) with catalogTable overload [Spark] Replaces startTransaction(Snapshot) calls with catalogTable overload Sep 29, 2023
Copy link
Collaborator

@ryan-johnson-databricks ryan-johnson-databricks left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -612,7 +612,7 @@ case class CreateDeltaTableCommand(
deltaLog: DeltaLog,
tableWithLocation: CatalogTable,
snapshotOpt: Option[Snapshot] = None): OptimisticTransaction = {
val txn = deltaLog.startTransaction(snapshotOpt)
val txn = deltaLog.startTransaction(None, snapshotOpt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use a comment why we don't pass tableWithLocation. I believe the reason is, tableWithLocation is the proposed catalog table entry, which will be installed by this command but does not yet exist as-of the start of the transaction?

scottsand-db pushed a commit that referenced this pull request Oct 6, 2023
…verload

This PR replaces all calls to DeltaLog.startTransaction(Snapshot) with calls to DeltaLog.startTransaction(Option[CatalogTable], Snapshot). This PR is part of #2105 and a follow-up to #2125. It makes sure that transactions have a valid catalogTable attached to them so Uniform can correctly update the table in the catalog.

This is a small refactoring change so existing test coverage is sufficient.

No

Closes #2126

GitOrigin-RevId: d82787c64979a2dd4a363bf92a1640b7635ec02e
xupefei pushed a commit to xupefei/delta that referenced this pull request Oct 31, 2023
…verload

This PR replaces all calls to DeltaLog.startTransaction(Snapshot) with calls to DeltaLog.startTransaction(Option[CatalogTable], Snapshot). This PR is part of delta-io#2105 and a follow-up to delta-io#2125. It makes sure that transactions have a valid catalogTable attached to them so Uniform can correctly update the table in the catalog.

This is a small refactoring change so existing test coverage is sufficient.

No

Closes delta-io#2126

GitOrigin-RevId: d82787c64979a2dd4a363bf92a1640b7635ec02e
@LukasRupprecht LukasRupprecht deleted the replace-start-transaction-snapshot branch April 3, 2024 00:19
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