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

Add get table api #210

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Add get table api #210

merged 2 commits into from
Nov 16, 2022

Conversation

andyl-db
Copy link
Collaborator

@andyl-db andyl-db commented Nov 11, 2022

This PR:

  • adds a get table api
  • changes the route for retrieving the version to /share/{share}/schema/{schema}/table/{table}/version due to conflicts with the get table api
  • upgrades dependencies for use on M1 macbook pro*
  • test/testQuietly to integrationTest to fix tests expecting an integration test

(*) the current version of the scalapb library does not use a version of protoc that has publicly available arm64 builds and fails the local development

@zhuansunxt zhuansunxt self-requested a review November 14, 2022 17:50
@linzhou-db linzhou-db self-requested a review November 14, 2022 21:46
Copy link
Collaborator

@linzhou-db linzhou-db left a comment

Choose a reason for hiding this comment

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

PROTOCOL.md Outdated Show resolved Hide resolved
project/plugins.sbt Show resolved Hide resolved
@Param("table") table: String
): GetTableResponse = processRequest {
sharedTableManager.getTable(share, schema, table)
GetTableResponse(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zhuansunxt
https://github.com/delta-io/delta-sharing/blob/main/PROTOCOL.md#list-tables-in-a-schema
In order to perform metadata reconciliation, we updated the delta sharing protocol and databricks server to return share id and table id for each table, but didn't update the oss code, this works as there's no o2d sharing.
We probably need to do this in a separate PR. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm not too worried about the O2D case. What we put here is just a reference implementation anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too worried about the actual O2D case either.

Just feel the code change should respect the protocol, unless there's some specific reason not to do so, such as too much effort, or some other tech difficulties, etc.

Copy link
Collaborator

@zhuansunxt zhuansunxt left a comment

Choose a reason for hiding this comment

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

Please work with @linzhou-db to see if protocol.md change should be included in this PR. I used to include everything in a single PR: #97

PROTOCOL.md Outdated Show resolved Hide resolved
@linzhou-db
Copy link
Collaborator

when do you plan to merge this?

@andyl-db andyl-db merged commit cb49591 into delta-io:main Nov 16, 2022
@andyl-db andyl-db deleted the SC-111517-get-table-api branch November 16, 2022 18:33
andyl-db added a commit that referenced this pull request Nov 18, 2022
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