-
Notifications
You must be signed in to change notification settings - Fork 151
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
Delta Sharing Server to support return response in delta log format #335
Conversation
|
||
/** | ||
* DeltaAddFile used in delta sharing protocol, copied from AddFile in delta. | ||
* Adding 4 fields: id/version/timestamp/expirationTimestamp. |
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.
What do these 4 field corresponds to?
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.
These are delta sharing related fields.
Added more comment.
/** | ||
* DeltaAddFile used in delta sharing protocol, copied from AddFile in delta. | ||
* Adding 4 fields: id/version/timestamp/expirationTimestamp. | ||
* Ignoring 1 field: tags. |
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.
Seems like we are missing more fields (not just tags). Could we document them as well?
deletionVector
, baseRowId
, defaultRowCommitVersion
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: are these fields all require minReaderVersion > 2?
If so we would skip them in the oss server change for now, since this change focuses on responseFormat=delta, we could add another PR to support DV, etc.
server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala
Outdated
Show resolved
Hide resolved
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/main/scala/io/delta/standalone/internal/DeltaSharedTableLoader.scala
Show resolved
Hide resolved
server/src/main/scala/io/delta/standalone/internal/DeltaSharedTableLoader.scala
Show resolved
Hide resolved
server/src/main/scala/io/delta/standalone/internal/DeltaSharedTableLoader.scala
Show resolved
Hide resolved
@@ -560,6 +568,10 @@ object DeltaSharingService { | |||
endingTimestamp.map(DeltaDataSource.CDF_END_TIMESTAMP_KEY -> _)).toMap | |||
} | |||
|
|||
private[server] def getResponseFormat(headerCapabilities: Map[String, String]): String = { | |||
headerCapabilities.get(DELTA_SHARING_RESPONSE_FORMAT).getOrElse("parquet") |
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.
In the current change, these keywords "parquet" and "delta" are hard coded everywhere.
Can we define them as constants in a central place and refer to them from everywhere (including tests) ?
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.
updated.
Delta Sharing Server to support return response in delta log format, 2 main changes:
Delta*
actions in model.scala, used when the requested format isdelta
.delta-sharing-capabilities
header in request and handle the requested format.This is part 1 for issue #341
original prototype in #298.