chitralverma commented on code in PR #7883:
URL: https://github.com/apache/opendal/pull/7883#discussion_r3544222598
##########
dev/src/generate/python.rs:
##########
@@ -244,3 +298,280 @@ fn make_python_type(ty: ViaDeserialize<ConfigType>) ->
Result<String, minijinja:
}
.to_string())
}
+
+/// Classifies a config field to drive codegen.
+#[derive(Clone, Copy, PartialEq)]
+enum FieldKind {
+ Str,
+ StrRequired,
+ Path,
+ Bool,
+ Num,
+ Vec,
+ Map,
+ Duration,
+ /// A core type the parser can't map (e.g. an enum); passed through as a
string.
+ Other,
+}
+
+fn field_kind(field: &Config) -> FieldKind {
+ match field.value {
+ ConfigType::Bool => FieldKind::Bool,
+ ConfigType::Usize | ConfigType::U64 | ConfigType::I64 |
ConfigType::U32 | ConfigType::U16 => {
+ FieldKind::Num
+ }
+ ConfigType::Vec => FieldKind::Vec,
+ ConfigType::HashMap => FieldKind::Map,
+ ConfigType::Duration => FieldKind::Duration,
+ ConfigType::String => {
+ if is_opaque_string_fallback(&field.name) {
+ FieldKind::Other
+ } else if is_path_like_field(&field.name) {
+ FieldKind::Path
+ } else if field.optional {
+ FieldKind::Str
+ } else {
+ FieldKind::StrRequired
+ }
+ }
+ }
+}
+
+/// Config fields whose core type is a string-parsed enum, not a `String`.
+fn is_opaque_string_fallback(field: &str) -> bool {
+ matches!(field, "repo_type" | "download_mode")
+}
+
+fn int_type(ty: ConfigType) -> &'static str {
+ match ty {
+ ConfigType::Usize => "usize",
+ ConfigType::U64 => "u64",
+ ConfigType::I64 => "i64",
+ ConfigType::U32 => "u32",
+ ConfigType::U16 => "u16",
+ _ => "usize",
+ }
+}
+
+/// The Rust `#[new]` parameter type, chosen so introspection renders the exact
+/// Python type.
+fn config_param_type(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ Ok(match field_kind(&field) {
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other =>
"Option<String>".to_string(),
+ FieldKind::StrRequired => "String".to_string(),
+ FieldKind::Path => "Option<crate::PyPath>".to_string(),
+ FieldKind::Bool => "Option<bool>".to_string(),
+ FieldKind::Num => format!("Option<{}>", int_type(field.value)),
+ FieldKind::Vec => "Option<Vec<String>>".to_string(),
+ FieldKind::Map => "Option<std::collections::HashMap<String,
String>>".to_string(),
+ })
+}
+
+/// The `#[new]` signature parameter, defaulting optional fields to `None`.
+fn config_new_param(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ if field_is_optional(&field) {
+ Ok(format!("{} = None", field.name))
+ } else {
+ Ok(field.name.to_string())
+ }
+}
+
+/// Insert a `#[new]` parameter into the option map handed to `from_iter`.
+fn config_to_opts(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => format!("opts.insert(\"{key}\".to_string(),
{name});"),
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other => {
+ format!("if let Some(v) = {name} {{
opts.insert(\"{key}\".to_string(), v); }}")
+ }
+ FieldKind::Path => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.into_string()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(), if
v {{ \"true\" }} else {{ \"false\" }}.to_string()); }}"
+ ),
+ FieldKind::Num => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.to_string()); }}"
+ ),
+ FieldKind::Vec => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.join(\",\")); }}"
+ ),
+ // Map fields are set on the config directly after `from_iter`.
+ FieldKind::Map => String::new(),
+ })
+}
+
+/// Set a map-valued field on the core config after `from_iter` (map values
+/// cannot pass through the flat option map). No-op for other kinds.
+fn config_assign_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::Map => format!("if let Some(v) = {name} {{ cfg.{name} =
Some(v); }}"),
+ _ => String::new(),
+ })
+}
+
+/// A getter (with doc comment) for a config field. Opaque fields have none.
+fn config_getter(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let kind = field_kind(&field);
+
+ if kind == FieldKind::Other {
+ return Ok(String::new());
+ }
+
+ let body = match kind {
+ FieldKind::StrRequired => {
+ format!("#[getter]\n fn {name}(&self) -> String {{
self.0.{name}.clone() }}")
+ }
+ FieldKind::Str => {
+ format!("#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.clone() }}")
+ }
+ // `PyPath`/`Option<bool>` keep the getter type equal to the
constructor.
+ FieldKind::Path => format!(
+ "#[getter]\n fn {name}(&self) -> Option<crate::PyPath> {{
self.0.{name}.clone().map(crate::PyPath::from) }}"
+ ),
+ FieldKind::Bool => {
+ format!("#[getter]\n fn {name}(&self) -> Option<bool> {{
Some(self.0.{name}) }}")
+ }
+ FieldKind::Num => {
+ let ty = int_type(field.value);
+ if field.optional {
+ format!("#[getter]\n fn {name}(&self) -> Option<{ty}> {{
self.0.{name} }}")
+ } else {
+ format!("#[getter]\n fn {name}(&self) -> {ty} {{
self.0.{name} }}")
+ }
+ }
+ FieldKind::Vec => {
+ format!(
+ "#[getter]\n fn {name}(&self) -> Option<Vec<String>> {{
self.0.{name}.clone() }}"
+ )
+ }
+ FieldKind::Map => format!(
+ "#[getter]\n fn {name}(&self) ->
Option<std::collections::HashMap<String, String>> {{ self.0.{name}.clone() }}"
+ ),
+ FieldKind::Duration => format!(
+ "#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.map(|d| format!(\"{{}}s\", d.as_secs())) }}"
+ ),
Review Comment:
Correct that `as_secs()` drops sub-second precision. It is currently
unreachable, though: core cannot deserialize a `Duration` config field from the
string map at all (it rejects every string with `invalid type: string, expected
struct Duration`), so no `Duration` value round-trips today. Tracked in #7887.
This formatting should move to a lossless representation once that core issue
is fixed.
##########
dev/src/generate/python.rs:
##########
@@ -244,3 +298,280 @@ fn make_python_type(ty: ViaDeserialize<ConfigType>) ->
Result<String, minijinja:
}
.to_string())
}
+
+/// Classifies a config field to drive codegen.
+#[derive(Clone, Copy, PartialEq)]
+enum FieldKind {
+ Str,
+ StrRequired,
+ Path,
+ Bool,
+ Num,
+ Vec,
+ Map,
+ Duration,
+ /// A core type the parser can't map (e.g. an enum); passed through as a
string.
+ Other,
+}
+
+fn field_kind(field: &Config) -> FieldKind {
+ match field.value {
+ ConfigType::Bool => FieldKind::Bool,
+ ConfigType::Usize | ConfigType::U64 | ConfigType::I64 |
ConfigType::U32 | ConfigType::U16 => {
+ FieldKind::Num
+ }
+ ConfigType::Vec => FieldKind::Vec,
+ ConfigType::HashMap => FieldKind::Map,
+ ConfigType::Duration => FieldKind::Duration,
+ ConfigType::String => {
+ if is_opaque_string_fallback(&field.name) {
+ FieldKind::Other
+ } else if is_path_like_field(&field.name) {
+ FieldKind::Path
+ } else if field.optional {
+ FieldKind::Str
+ } else {
+ FieldKind::StrRequired
+ }
+ }
+ }
+}
+
+/// Config fields whose core type is a string-parsed enum, not a `String`.
+fn is_opaque_string_fallback(field: &str) -> bool {
+ matches!(field, "repo_type" | "download_mode")
+}
+
+fn int_type(ty: ConfigType) -> &'static str {
+ match ty {
+ ConfigType::Usize => "usize",
+ ConfigType::U64 => "u64",
+ ConfigType::I64 => "i64",
+ ConfigType::U32 => "u32",
+ ConfigType::U16 => "u16",
+ _ => "usize",
+ }
+}
+
+/// The Rust `#[new]` parameter type, chosen so introspection renders the exact
+/// Python type.
+fn config_param_type(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ Ok(match field_kind(&field) {
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other =>
"Option<String>".to_string(),
+ FieldKind::StrRequired => "String".to_string(),
+ FieldKind::Path => "Option<crate::PyPath>".to_string(),
+ FieldKind::Bool => "Option<bool>".to_string(),
+ FieldKind::Num => format!("Option<{}>", int_type(field.value)),
+ FieldKind::Vec => "Option<Vec<String>>".to_string(),
+ FieldKind::Map => "Option<std::collections::HashMap<String,
String>>".to_string(),
+ })
+}
+
+/// The `#[new]` signature parameter, defaulting optional fields to `None`.
+fn config_new_param(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ if field_is_optional(&field) {
+ Ok(format!("{} = None", field.name))
+ } else {
+ Ok(field.name.to_string())
+ }
+}
+
+/// Insert a `#[new]` parameter into the option map handed to `from_iter`.
+fn config_to_opts(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => format!("opts.insert(\"{key}\".to_string(),
{name});"),
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other => {
+ format!("if let Some(v) = {name} {{
opts.insert(\"{key}\".to_string(), v); }}")
+ }
+ FieldKind::Path => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.into_string()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(), if
v {{ \"true\" }} else {{ \"false\" }}.to_string()); }}"
+ ),
+ FieldKind::Num => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.to_string()); }}"
+ ),
+ FieldKind::Vec => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.join(\",\")); }}"
+ ),
+ // Map fields are set on the config directly after `from_iter`.
+ FieldKind::Map => String::new(),
+ })
+}
+
+/// Set a map-valued field on the core config after `from_iter` (map values
+/// cannot pass through the flat option map). No-op for other kinds.
+fn config_assign_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::Map => format!("if let Some(v) = {name} {{ cfg.{name} =
Some(v); }}"),
+ _ => String::new(),
+ })
+}
+
+/// A getter (with doc comment) for a config field. Opaque fields have none.
+fn config_getter(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let kind = field_kind(&field);
+
+ if kind == FieldKind::Other {
+ return Ok(String::new());
+ }
+
+ let body = match kind {
+ FieldKind::StrRequired => {
+ format!("#[getter]\n fn {name}(&self) -> String {{
self.0.{name}.clone() }}")
+ }
+ FieldKind::Str => {
+ format!("#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.clone() }}")
+ }
+ // `PyPath`/`Option<bool>` keep the getter type equal to the
constructor.
+ FieldKind::Path => format!(
+ "#[getter]\n fn {name}(&self) -> Option<crate::PyPath> {{
self.0.{name}.clone().map(crate::PyPath::from) }}"
+ ),
+ FieldKind::Bool => {
+ format!("#[getter]\n fn {name}(&self) -> Option<bool> {{
Some(self.0.{name}) }}")
+ }
+ FieldKind::Num => {
+ let ty = int_type(field.value);
+ if field.optional {
+ format!("#[getter]\n fn {name}(&self) -> Option<{ty}> {{
self.0.{name} }}")
+ } else {
+ format!("#[getter]\n fn {name}(&self) -> {ty} {{
self.0.{name} }}")
+ }
+ }
+ FieldKind::Vec => {
+ format!(
+ "#[getter]\n fn {name}(&self) -> Option<Vec<String>> {{
self.0.{name}.clone() }}"
+ )
+ }
+ FieldKind::Map => format!(
+ "#[getter]\n fn {name}(&self) ->
Option<std::collections::HashMap<String, String>> {{ self.0.{name}.clone() }}"
+ ),
+ FieldKind::Duration => format!(
+ "#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.map(|d| format!(\"{{}}s\", d.as_secs())) }}"
+ ),
+ FieldKind::Other => unreachable!(),
+ };
+
+ let doc = config_field_doc(field)?;
+ if doc.is_empty() {
+ Ok(body)
+ } else {
+ Ok(format!("{doc}\n {body}"))
+ }
+}
+
+/// Insert a field into the flat option map used for `repr`/pickle.
+fn config_to_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => {
+ format!("map.insert(\"{key}\".to_string(),
self.0.{name}.clone());")
+ }
+ FieldKind::Str | FieldKind::Path => format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.clone()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "map.insert(\"{key}\".to_string(), if self.0.{name} {{ \"true\" }}
else {{ \"false\" }}.to_string());"
+ ),
+ FieldKind::Num => {
+ if field.optional {
+ format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.to_string()); }}"
+ )
+ } else {
+ format!("map.insert(\"{key}\".to_string(),
self.0.{name}.to_string());")
+ }
+ }
+ FieldKind::Vec => format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.join(\",\")); }}"
+ ),
+ FieldKind::Duration => format!(
+ "if let Some(d) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), format!(\"{{}}s\", d.as_secs())); }}"
+ ),
Review Comment:
Same as the getter case: `as_secs()` is lossy, but the flat map never
carries a real `Duration` value because core cannot parse `Duration` config
fields from strings (see #7887). No behavior change on pickle round-trip is
reachable today. Worth revisiting the representation together with the core fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
