feat: serialize storage options in table identifier proto#5973
feat: serialize storage options in table identifier proto#5973LuQQiu merged 4 commits intolance-format:mainfrom
Conversation
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
Code ReviewP0 - Security Concern: Credentials in Serialized ProtoThe addition of
These are now being serialized into the proto message for distributed execution (planner → executor). This could expose credentials if:
Suggestions:
If this is intentional for specific distributed execution use cases with proper security controls, please document the expected security model. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| manifest_etag: dataset.manifest_location.e_tag.clone(), | ||
| serialized_manifest: Some(manifest_proto.encode_to_vec()), | ||
| storage_options: dataset | ||
| .initial_storage_options() |
There was a problem hiding this comment.
I think this should call latest_storage_options()?
| // Two modes: | ||
| // 1. uri + serialized_manifest (fast): remote executor skips manifest read. | ||
| // 2. uri + version + etag (lightweight): remote executor loads manifest from storage. | ||
| message TableIdentifier { |
There was a problem hiding this comment.
I feel at this point it's more like a table state than table identifier, not sure if we can still change ethe name though
There was a problem hiding this comment.
emmm, we mention not make the proto backward compatible, this may be merged within one week... emm
jackye1995
left a comment
There was a problem hiding this comment.
2 comments, other parts looks good to me
add storage options in table identifier proto to allow pass in storage credentials or information.
Make
datasetparameter optional infiltered_read_exec_from_proto, falls back to opening from the proto's table identifier