Skip to content

Commit 7bddfee

Browse files
gbrgrgbrgr
committed
PR comment
1 parent 9213bcd commit 7bddfee

4 files changed

Lines changed: 62 additions & 64 deletions

File tree

‎crates/catalog/rest/src/catalog.rs‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3248,14 +3248,19 @@ mod tests {
32483248

32493249
// Verify that the FileIO was constructed with the correct runtime context so that
32503250
// RefreshableStorageFactory::build() will receive it when first invoked for I/O.
3251-
let file_io_config = table.file_io().config();
3252-
assert_eq!(file_io_config.table_ident(), Some(&expected_ident));
3251+
let props = table.file_io().config().props();
3252+
let expected_location = "s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json";
32533253
assert_eq!(
3254-
file_io_config.location(),
3255-
Some(
3256-
"s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json"
3257-
)
3258-
);
3254+
props
3255+
.get("iceberg.internal.metadata-location")
3256+
.map(String::as_str),
3257+
Some(expected_location),
3258+
);
3259+
let ident_json = props
3260+
.get("iceberg.internal.table-ident")
3261+
.expect("table-ident prop should be set");
3262+
let parsed_ident: TableIdent = serde_json::from_str(ident_json).unwrap();
3263+
assert_eq!(parsed_ident, expected_ident);
32593264

32603265
config_mock.assert_async().await;
32613266
load_table_mock.assert_async().await;

‎crates/iceberg/src/io/file_io.rs‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use std::sync::{Arc, OnceLock};
2020

2121
use bytes::Bytes;
2222

23+
use super::storage::config::{PROP_METADATA_LOCATION, PROP_TABLE_IDENT};
2324
use super::storage::{
2425
LocalFsStorageFactory, MemoryStorageFactory, Storage, StorageConfig, StorageFactory,
2526
};
@@ -206,15 +207,27 @@ impl FileIOBuilder {
206207
self
207208
}
208209

209-
/// Set the runtime table identity context (passed to `StorageFactory::build`).
210+
/// Set the runtime table identity context.
211+
///
212+
/// Serializes the `TableIdent` as JSON into a well-known prop key so that
213+
/// `RefreshableStorageFactory::build()` can recover it and pass it to the
214+
/// credential loader on refresh. Other storage factories ignore this prop.
210215
pub fn with_table_ident(mut self, table_ident: crate::catalog::TableIdent) -> Self {
211-
self.config = self.config.with_table_ident(table_ident);
216+
let json =
217+
serde_json::to_string(&table_ident).expect("TableIdent serialization is infallible");
218+
self.config = self.config.with_prop(PROP_TABLE_IDENT, json);
212219
self
213220
}
214221

215-
/// Set the runtime metadata location context (passed to `StorageFactory::build`).
222+
/// Set the runtime metadata location context.
223+
///
224+
/// Stores the location under a well-known prop key so that
225+
/// `RefreshableStorageFactory::build()` can pass it to the credential loader on refresh.
226+
/// Other storage factories ignore this prop.
216227
pub fn with_location(mut self, location: impl Into<String>) -> Self {
217-
self.config = self.config.with_location(location);
228+
self.config = self
229+
.config
230+
.with_prop(PROP_METADATA_LOCATION, location.into());
218231
self
219232
}
220233

‎crates/iceberg/src/io/storage/config/mod.rs‎

Lines changed: 8 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,13 @@ pub use oss::*;
4343
pub use s3::*;
4444
use serde::{Deserialize, Serialize};
4545

46-
use crate::catalog::TableIdent;
46+
/// Prop key under which the JSON-serialized `TableIdent` is stored in `StorageConfig::props`.
47+
/// Consumed and stripped by `RefreshableStorageFactory::build()`; ignored by all other factories.
48+
pub(crate) const PROP_TABLE_IDENT: &str = "iceberg.internal.table-ident";
49+
50+
/// Prop key under which the metadata location string is stored in `StorageConfig::props`.
51+
/// Consumed and stripped by `RefreshableStorageFactory::build()`; ignored by all other factories.
52+
pub(crate) const PROP_METADATA_LOCATION: &str = "iceberg.internal.metadata-location";
4753

4854
/// Configuration properties for storage backends.
4955
///
@@ -55,26 +61,13 @@ use crate::catalog::TableIdent;
5561
pub struct StorageConfig {
5662
/// Configuration properties for the storage backend
5763
props: HashMap<String, String>,
58-
59-
/// Runtime context: the table being accessed. Not serialized; set by the
60-
/// catalog at table-load time so that `StorageFactory::build()` implementations
61-
/// (e.g. `RefreshableStorageFactory`) can pass it to the credential loader.
62-
#[serde(skip)]
63-
pub(crate) table_ident: Option<TableIdent>,
64-
65-
/// Runtime context: the metadata location of the table being accessed.
66-
/// Not serialized; set by the catalog at table-load time.
67-
#[serde(skip)]
68-
pub(crate) location: Option<String>,
6964
}
7065

7166
impl StorageConfig {
7267
/// Create a new empty StorageConfig.
7368
pub fn new() -> Self {
7469
Self {
7570
props: HashMap::new(),
76-
table_ident: None,
77-
location: None,
7871
}
7972
}
8073

@@ -84,33 +77,7 @@ impl StorageConfig {
8477
///
8578
/// * `props` - Configuration properties for the storage backend
8679
pub fn from_props(props: HashMap<String, String>) -> Self {
87-
Self {
88-
props,
89-
table_ident: None,
90-
location: None,
91-
}
92-
}
93-
94-
/// Set the runtime table identity context.
95-
pub fn with_table_ident(mut self, table_ident: TableIdent) -> Self {
96-
self.table_ident = Some(table_ident);
97-
self
98-
}
99-
100-
/// Set the runtime metadata location context.
101-
pub fn with_location(mut self, location: impl Into<String>) -> Self {
102-
self.location = Some(location.into());
103-
self
104-
}
105-
106-
/// Get the runtime table identity context, if set.
107-
pub fn table_ident(&self) -> Option<&TableIdent> {
108-
self.table_ident.as_ref()
109-
}
110-
111-
/// Get the runtime metadata location context, if set.
112-
pub fn location(&self) -> Option<&str> {
113-
self.location.as_deref()
80+
Self { props }
11481
}
11582

11683
/// Get all configuration properties.

‎crates/iceberg/src/io/storage/opendal/mod.rs‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ use url::Url;
4242

4343
use crate::catalog::{NamespaceIdent, TableIdent};
4444
use crate::io::refreshable_storage::RefreshableOpenDalStorageBuilder;
45+
use crate::io::storage::config::{PROP_METADATA_LOCATION, PROP_TABLE_IDENT};
4546
use crate::io::{
4647
FileMetadata, FileRead, FileWrite, InputFile, OutputFile, Storage, StorageConfig,
4748
StorageCredentialsLoader, StorageFactory,
@@ -544,8 +545,11 @@ mod tests {
544545
let factory = RefreshableStorageFactory::new(loader);
545546
let table_ident = TableIdent::from_strs(["ns", "tbl"]).unwrap();
546547
let config = StorageConfig::new()
547-
.with_location("s3://test-bucket/")
548-
.with_table_ident(table_ident);
548+
.with_prop(PROP_METADATA_LOCATION, "s3://test-bucket/")
549+
.with_prop(
550+
PROP_TABLE_IDENT,
551+
serde_json::to_string(&table_ident).unwrap(),
552+
);
549553
assert!(
550554
factory.build(&config).is_ok(),
551555
"RefreshableStorageFactory should build a Refreshable storage successfully"
@@ -560,8 +564,11 @@ mod tests {
560564
let table_ident = TableIdent::from_strs(["ns", "tbl"]).unwrap();
561565
// Initial credentials are merged into props before build() is called
562566
let config = StorageConfig::new()
563-
.with_location("s3://test-bucket/")
564-
.with_table_ident(table_ident);
567+
.with_prop(PROP_METADATA_LOCATION, "s3://test-bucket/")
568+
.with_prop(
569+
PROP_TABLE_IDENT,
570+
serde_json::to_string(&table_ident).unwrap(),
571+
);
565572
assert!(
566573
factory.build(&config).is_ok(),
567574
"RefreshableStorageFactory should build successfully"
@@ -614,20 +621,26 @@ impl StorageFactory for RefreshableStorageFactory {
614621
)
615622
})?;
616623

617-
let location = config.location().unwrap_or("").to_string();
624+
// Extract runtime context from props, stripping the internal keys so they
625+
// don't leak into the underlying OpenDAL operator configuration.
626+
let mut props = config.props().clone();
627+
let location = props.remove(PROP_METADATA_LOCATION).unwrap_or_default();
618628
let scheme = Url::parse(&location)
619629
.map(|u| u.scheme().to_string())
620630
.unwrap_or_default();
621-
let table_ident = config.table_ident().cloned().unwrap_or_else(|| {
622-
TableIdent::new(
623-
NamespaceIdent::new("unknown".to_string()),
624-
"unknown".to_string(),
625-
)
626-
});
631+
let table_ident = props
632+
.remove(PROP_TABLE_IDENT)
633+
.and_then(|s| serde_json::from_str::<TableIdent>(&s).ok())
634+
.unwrap_or_else(|| {
635+
TableIdent::new(
636+
NamespaceIdent::new("unknown".to_string()),
637+
"unknown".to_string(),
638+
)
639+
});
627640

628641
let backend = RefreshableOpenDalStorageBuilder::new()
629642
.scheme(scheme)
630-
.base_props(config.props().clone())
643+
.base_props(props)
631644
.credentials_loader(Arc::clone(loader))
632645
.location(location)
633646
.table_ident(table_ident)

0 commit comments

Comments
 (0)