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

OSS Delta Sharing Server: Adds api to accept cdf query #135

Merged
merged 9 commits into from May 13, 2022
Merged

Conversation

linzhou-db
Copy link
Collaborator

OSS Delta Sharing Server: Adds api to accept cdf query

  • @get("/shares/{share}/schemas/{schema}/tables/{table}/changes")
  • Parse url parameters and construct the cdfoptions map
  • Add classes DeltaErrors and DeltaDataSource for some exceptions and constants
  • Add DeltaSharedTable.queryCDF and return not implemented exception

@linzhou-db linzhou-db self-assigned this May 10, 2022
@zhuansunxt
Copy link
Collaborator

Hi @linzhou-db , could you have a standalone PR on https://github.com/delta-io/delta-sharing/blob/main/PROTOCOL.md with the new API's specification? It's better to agree on specification before adding its implementation on the server IMO.

@linzhou-db
Copy link
Collaborator Author

Hi @linzhou-db , could you have a standalone PR on https://github.com/delta-io/delta-sharing/blob/main/PROTOCOL.md with the new API's specification? It's better to agree on specification before adding its implementation on the server IMO.

sg, will do

@linzhou-db
Copy link
Collaborator Author

Hi @linzhou-db , could you have a standalone PR on https://github.com/delta-io/delta-sharing/blob/main/PROTOCOL.md with the new API's specification? It's better to agree on specification before adding its implementation on the server IMO.

sg, will do

Sent out #137

}
if (startingVersion.isDefined) {
try {
startingVersion.get.toLong
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we validate timestamp?
If it gets validate elsewhere, perhaps add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment, same as the code in our repo, it's validated in the cdc reader.

)
}

def noStartVersionForCDF: Throwable = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call it noStartParamForCDF?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I more prefer to keep it the same as DeltaErrors in our code base.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sg.

@linzhou-db
Copy link
Collaborator Author

@zhuansunxt you still want to take a look?

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.

LGTM

@linzhou-db linzhou-db merged commit cf943f9 into main May 13, 2022
@Basem-Gaber
Copy link

Why is the concept of table changes coupled with the CDF ?

Isn't it possible to extract the info of the difference between a version and another by only knowing which files were added and which were removed ?

The API can return this metadata which should be sufficient for the client to use to replicate the changes, correct ?

@linzhou-db
Copy link
Collaborator Author

Hi Basem,
Thanks for your interest and question.

Why is the concept of table changes coupled with the CDF ?

CDF is short for Change Data Feed, it IS the concept of table changes.

Isn't it possible to extract the info of the difference between a version and another by only knowing which files were added and which were removed ?

The API can return this metadata which should be sufficient for the client to use to replicate the changes, correct ?

Technically yes, while CDF is a well developed, organized, and well maintained feature for this purpose, so good to just use it.

@wchau wchau deleted the SC-100359 branch July 8, 2022 23:27
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.

None yet

4 participants