CTTY commented on code in PR #2988:
URL: https://github.com/apache/iceberg-rust/pull/2988#discussion_r3770797798
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -16,70 +16,25 @@
// under the License.
use std::collections::HashMap;
-use std::fmt::Display;
-use std::str::FromStr;
+
+use iceberg_property_macro::Properties;
use crate::compression::CompressionCodec;
use crate::error::{Error, ErrorKind, Result};
use crate::util::location::strip_trailing_slash;
-fn parse_property<T: FromStr>(
- properties: &HashMap<String, String>,
- key: &str,
- default: T,
-) -> Result<T>
-where
- <T as FromStr>::Err: Display,
-{
- properties.get(key).map_or(Ok(default), |value| {
- value.parse::<T>().map_err(|e| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Invalid value for {key}: {e}"),
- )
- })
- })
-}
-
-/// Parse an optional property, returning `None` when the key is absent and an
-/// error when the value is present but fails to parse.
-fn parse_optional_property<T: FromStr>(
- properties: &HashMap<String, String>,
- key: &str,
-) -> Result<Option<T>>
-where
- <T as FromStr>::Err: Display,
-{
- properties
- .get(key)
- .map(|value| {
- value.parse::<T>().map_err(|e| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Invalid value for {key}: {e}"),
- )
- })
- })
- .transpose()
-}
-
-fn parse_location_property(
- properties: &HashMap<String, String>,
- key: &str,
-) -> Result<Option<String>> {
- properties
- .get(key)
- .map(|path| {
- if path.is_empty() {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!("Invalid value for {key}: path must not be empty"),
- ));
- }
+fn parse_location_property(path: &str) -> Result<String> {
+ if path.is_empty() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Invalid value for {}: path must not be empty",
+ TableProperties::PROPERTY_WRITE_METADATA_PATH
Review Comment:
I think we should not hardcode this. Some other location properties like
`write_object_storage_location` could reuse this but they didn't do so. we
should fix it
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
