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
Support startingTimestamp and GET method for getTableVersion #199
Conversation
linzhou-db
commented
Oct 16, 2022
•
edited
edited
- Support startingTimestamp for getTableVersion
- Support GET http method for getTableVersion, and update DeltaSharingClient to use GET instead of HEAD
- Update both python and spark python client to issue GET instead of HEAD request for getTableVersion
- in python rest client, include table version in the response for list_files_in_table and query_table_metadata
- Check that queryTable with "timestamp" cannot exceeds TableConfig.startVersion
server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala
Outdated
Show resolved
Hide resolved
server/src/main/scala/io/delta/standalone/internal/DeltaSharedTableLoader.scala
Show resolved
Hide resolved
server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala
Show resolved
Hide resolved
@@ -240,12 +240,28 @@ class DeltaSharingService(serverConfig: ServerConfig) { | |||
} | |||
|
|||
@Head("/shares/{share}/schemas/{schema}/tables/{table}") | |||
@Get("/shares/{share}/schemas/{schema}/tables/{table}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there's no way to "not support startingTimestamp for HEAD" given this config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that oss server will support startingTimestamp for HEAD method but DB's server will not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can mention in the protocol doc that startingTimestamp is not supported for HEAD
headers = values[0] | ||
# it's a bug in the server if it doesn't return delta-table-version in the header | ||
if "delta-table-version" not in headers: | ||
raise LookupError("Missing delta-table-version header") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Can you remind me why we decided to return the table version in the header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I don't know. I guess it was for light request/response?
@zsxwing Do you still remember this?
@@ -240,12 +240,28 @@ class DeltaSharingService(serverConfig: ServerConfig) { | |||
} | |||
|
|||
@Head("/shares/{share}/schemas/{schema}/tables/{table}") | |||
@Get("/shares/{share}/schemas/{schema}/tables/{table}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that oss server will support startingTimestamp for HEAD method but DB's server will not?