Copilot commented on code in PR #598:
URL: https://github.com/apache/hudi-rs/pull/598#discussion_r3197883122
##########
crates/core/src/config/table.rs:
##########
@@ -211,15 +209,23 @@ impl ConfigParser for HudiTableConfig {
}
}
+ fn aliases(&self) -> &[ConfigAlias] {
+ match self {
+ Self::PrecombineField => {
+ const ALIASES: &[ConfigAlias] =
+ &[ConfigAlias::new("hoodie.table.ordering.fields")];
+ ALIASES
+ }
Review Comment:
`PrecombineField` is aliased to `hoodie.table.ordering.fields`, but the
parser for `PrecombineField` treats the resolved value as a single field name.
If `hoodie.table.ordering.fields` is provided as a comma-separated list (plural
name), the resolved value would include commas and downstream code that expects
a single column name will fail (e.g. schema/array lookup by field name).
Consider normalizing/validating the alias value (split+trim) and either
selecting a single field or returning a clear error when multiple fields are
specified.
##########
crates/core/src/config/mod.rs:
##########
@@ -52,11 +74,36 @@ pub trait ConfigParser: AsRef<str> {
self.as_ref().to_string()
}
+ /// Returns alternative keys for this configuration.
+ fn aliases(&self) -> &[ConfigAlias] {
+ &[]
+ }
+
/// To indicate if the configuration is required or not, this helps in
validation.
fn is_required(&self) -> bool {
false
}
+ /// Resolve the raw string value from configs, checking the primary key
first, then aliases.
+ fn resolve_raw_value<'a>(&self, configs: &'a HashMap<String, String>) ->
Result<&'a str> {
+ if let Some(v) = configs.get(self.as_ref()) {
+ return Ok(v.as_str());
+ }
+ for alias in self.aliases() {
+ if let Some(v) = configs.get(alias.key) {
+ if alias.deprecated {
+ log::warn!(
+ "Config '{}' is deprecated; use '{}' instead",
+ alias.key,
+ self.as_ref()
+ );
+ }
+ return Ok(v.as_str());
+ }
Review Comment:
`resolve_raw_value()` emits a deprecation `log::warn!` every time a
deprecated alias key is resolved. Since config lookups (e.g.
`hudi_configs.get(...)`) can occur repeatedly in execution paths, this can
flood logs and add overhead. Consider warning only once per (alias.key, primary
key) pair (e.g. via a static set/OnceLock), or moving the warning to a one-time
validation/initial parse step rather than on every lookup.
--
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]
