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

[Kernel] Resolve path given to Table.forPath using TableClient APIs #2064

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

vkorukanti
Copy link
Collaborator

@vkorukanti vkorukanti commented Sep 15, 2023

Which Delta project/connector is this regarding?

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

Description

Currently, we expect the path given to Table.forPath() to be fully qualified. This enforces an unnecessary burden on the connector to add the necessary schema or authority etc. Instead, add a FileSystemClient.resolvePath API and use it to resolve the path to a fully qualified path from Table.forPath().

How was this patch tested?

Existing tests (deleted the file: prefix added to tests tables in the path) and a couple of new tests around missing table paths.

Copy link
Collaborator

@allisonport-db allisonport-db left a comment

Choose a reason for hiding this comment

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

Can we test the check in SnapshotManager? Like

  1. create Table.forPath(...)
  2. delete that directory
  3. call getLatestSnapshot()

This is also coincidentally one of the tests I saw in the standalone suites that I noted down to copy over.

@@ -68,6 +68,11 @@ trait TestUtils extends Assertions {
}
}

def latestSnapshot(path: String): Snapshot = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@scottsand-db didn't we discuss explicitly not using this as a test API?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i revert my opinion; im fine with this; thanks!

@allisonport-db
Copy link
Collaborator

Standalone test is here https://github.com/delta-io/delta/blob/master/connectors/standalone/src/test/scala/io/delta/standalone/internal/DeltaLogSuite.scala#L159 if that helps

Copy link
Collaborator

@allisonport-db allisonport-db left a comment

Choose a reason for hiding this comment

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

LGTM

@vkorukanti vkorukanti merged commit 701520b into delta-io:master Sep 15, 2023
6 of 7 checks passed
@vkorukanti vkorukanti deleted the resolvePath2 branch October 2, 2023 05:17
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.

3 participants