You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "kazdy (via GitHub)" <gi...@apache.org> on 2024/10/11 23:37:17 UTC
[PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
kazdy opened a new pull request, #163:
URL: https://github.com/apache/hudi-rs/pull/163
## Description
<!--- Describe your changes in detail -->
<!--- If it fixes an open issue, please link to the issue here. -->
<!--- Please link any related issues and PRs as well. -->
## How are the changes test-covered
- [ ] N/A
- [ ] Automated tests (unit and/or integration tests)
- [ ] Manual tests
- [ ] Details are described below
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#issuecomment-2408281692
@xushiyan please let me know if this is a direction you're fine with
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1798538422
##########
python/tests/test_table_read.py:
##########
Review Comment:
yup good improvement
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797630157
##########
python/tests/test_table_read.py:
##########
@@ -133,7 +133,9 @@ def test_sample_table(get_sample_table):
},
]
- table = HudiTable(table_path, {"hoodie.read.as.of.timestamp": "20240402123035233"})
+ table = HudiTable(
+ table_path, hudi_options={"hoodie.read.as.of.timestamp": "20240402123035233"}
+ )
Review Comment:
ditto
##########
python/src/internal.rs:
##########
@@ -101,12 +102,18 @@ pub struct HudiTable {
#[pymethods]
impl HudiTable {
#[new]
- #[pyo3(signature = (table_uri, options = None))]
- fn new(table_uri: &str, options: Option<HashMap<String, String>>) -> PyResult<Self> {
- let _table = rt().block_on(Table::new_with_options(
- table_uri,
- options.unwrap_or_default(),
- ))?;
+ #[pyo3(signature = (table_uri, hudi_options = None, storage_options = None))]
+ fn new(
+ table_uri: &str,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
Review Comment:
this can be reverted as well
##########
python/hudi/_internal.pyi:
##########
@@ -38,7 +38,8 @@ class HudiTable:
def __init__(
self,
table_uri: str,
- options: Optional[Dict[str, str]] = None,
+ hudi_options: Optional[Dict[str, str]] = None,
+ storage_options: Optional[Dict[str, str]] = None,
Review Comment:
you can revert this change - we keep python and rust api as close as possible
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` API for passing options [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan merged PR #163:
URL: https://github.com/apache/hudi-rs/pull/163
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1798552814
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::table::HudiTableConfig;
+use crate::config::utils::{parse_data_for_options, split_hudi_options_from_others};
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+
+#[derive(Debug, Clone)]
+pub struct TableBuilder {
+ base_uri: String,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_base_uri(base_uri: &str) -> Self {
+ TableBuilder {
+ base_uri: base_uri.to_string(),
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+
+ pub fn with_options<I, K, V>(self, all_options: I) -> Self
+ where
+ I: IntoIterator<Item = (K, V)>,
+ K: AsRef<str>,
+ V: Into<String>,
+ {
+ let mut hudi_options = HashMap::new();
+ let mut storage_options = HashMap::new();
+
+ let (hudi_opts, others) = split_hudi_options_from_others(all_options);
+ hudi_options.extend(hudi_opts);
+ storage_options.extend(others);
+
+ self.with_hudi_options(hudi_options)
+ .with_storage_options(storage_options)
+ }
+
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ match self.hudi_options {
+ None => self.hudi_options = Some(hudi_options),
+ Some(options) => self.hudi_options = Some(Self::merge(options, hudi_options)),
+ }
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ match self.storage_options {
+ None => self.storage_options = Some(storage_options),
+ Some(options) => self.storage_options = Some(Self::merge(options, storage_options)),
+ }
+ self
+ }
+
+ pub async fn build(self) -> anyhow::Result<Table> {
+ let hudi_options = self.hudi_options.unwrap_or_default().clone();
+ let mut storage_options = self.storage_options.unwrap_or_default().clone();
+
+ Self::load_storage_options(&mut storage_options);
+
+ let hudi_configs =
+ Self::load_hudi_configs(self.base_uri.clone(), hudi_options, &storage_options)
+ .await
+ .context("Failed to load table properties")?;
+
+ let hudi_configs = Arc::from(hudi_configs);
+ let storage_options = Arc::from(storage_options);
+
+ let timeline = Timeline::new(hudi_configs.clone(), storage_options.clone())
+ .await
+ .context("Failed to load timeline")?;
+
+ let file_system_view = FileSystemView::new(hudi_configs.clone(), storage_options.clone())
+ .await
+ .context("Failed to load file system view")?;
+
+ Ok(Table {
+ hudi_configs,
+ storage_options,
+ timeline,
+ file_system_view,
+ })
+ }
+
+ fn load_storage_options(storage_options: &mut HashMap<String, String>) {
+ Self::imbue_cloud_env_vars(storage_options);
+ }
+
+ async fn load_hudi_configs(
+ base_uri: String,
+ mut hudi_options: HashMap<String, String>,
+ storage_options: &HashMap<String, String>,
+ ) -> anyhow::Result<HudiConfigs> {
+ hudi_options.insert(
+ HudiTableConfig::BasePath.as_ref().to_string(),
+ base_uri.to_string(),
+ );
+
+ // create a [Storage] instance to load properties from storage layer.
+ let storage = Storage::new(
+ Arc::new(storage_options.clone()),
+ Arc::new(HudiConfigs::new(&hudi_options)),
+ )?;
+
+ Self::imbue_table_properties(&mut hudi_options, storage.clone()).await?;
+
+ Self::imbue_global_hudi_configs_if_not_present(&mut hudi_options, storage.clone()).await?;
+
+ let hudi_configs = HudiConfigs::new(hudi_options);
+
+ Table::validate_configs(&hudi_configs).expect("Hudi configs are not valid.");
+ Ok(hudi_configs)
+ }
+
+ fn imbue_cloud_env_vars(options: &mut HashMap<String, String>) {
+ const PREFIXES: [&str; 3] = ["AWS_", "AZURE_", "GOOGLE_"];
+
+ for (key, value) in env::vars() {
+ if PREFIXES.iter().any(|prefix| key.starts_with(prefix))
+ && !options.contains_key(&key.to_ascii_lowercase())
+ {
+ options.insert(key.to_ascii_lowercase(), value);
+ }
+ }
+ }
+
+ async fn imbue_table_properties(
+ options: &mut HashMap<String, String>,
+ storage: Arc<Storage>,
+ ) -> anyhow::Result<()> {
+ let bytes = storage.get_file_data(".hoodie/hoodie.properties").await?;
+ let table_properties = parse_data_for_options(&bytes, "=")?;
+
+ // TODO: handle the case where the same key is present in both table properties and options
Review Comment:
it's a bit complicated, there are difference cases pertaining to individual config - i'll fix this todo
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#issuecomment-2408508988
> @kazdy i'm cutting release branch within 1 day. do you think you can update this soon?
yes, I'll give it a try
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797643370
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
Ok I'll do it, but I'd like to still give users (and myself as a user as well) option to be specific.
I wanted to have storage_options because from the first look I did not figure out options handle both hudi and storage opts.
So it'll be
TableBuilder::with_options() -> handle generic options
TableBuilder::with_hudi_options()
TableBuilder::with_storage_options()
I hope that's fine
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#issuecomment-2408817456
@kazdy I'll push some fixes and then get this landed for release
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797684388
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
but still, builder pattern usually does validation in the end when call build(), we don't want to panic during the building phase of with_xxx(). One point i can think of is, making input taken from explicit APIs have higher precedence than those taken by generic ones. This should be handled in build()
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797756672
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
> I'm not sure if I'll be able to add more validation now, but I can give it a try in the next PR?
yup sounds good
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797683760
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
if you add additional checks, like for hudi_options, do some validation (there is a util function for this) for the input, and for storage options, check they have certain prefixes, then that can be helpful in preventing mistakes
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1798274404
##########
python/tests/test_table_read.py:
##########
Review Comment:
@xushiyan please take a look, I took the freedom to split this one big test to a few smaller ones, I hope that's fine with you
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1798273987
##########
python/tests/test_table_builder.py:
##########
Review Comment:
@xushiyan please take a look
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1798370147
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::table::HudiTableConfig;
+use crate::config::utils::{parse_data_for_options, split_hudi_options_from_others};
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+
+#[derive(Debug, Clone)]
+pub struct TableBuilder {
+ base_uri: String,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_base_uri(base_uri: &str) -> Self {
+ TableBuilder {
+ base_uri: base_uri.to_string(),
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+
+ pub fn with_options<I, K, V>(self, all_options: I) -> Self
+ where
+ I: IntoIterator<Item = (K, V)>,
+ K: AsRef<str>,
+ V: Into<String>,
+ {
+ let mut hudi_options = HashMap::new();
+ let mut storage_options = HashMap::new();
+
+ let (hudi_opts, others) = split_hudi_options_from_others(all_options);
+ hudi_options.extend(hudi_opts);
+ storage_options.extend(others);
+
+ self.with_hudi_options(hudi_options)
+ .with_storage_options(storage_options)
+ }
+
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ match self.hudi_options {
+ None => self.hudi_options = Some(hudi_options),
+ Some(options) => self.hudi_options = Some(Self::merge(options, hudi_options)),
+ }
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ match self.storage_options {
+ None => self.storage_options = Some(storage_options),
+ Some(options) => self.storage_options = Some(Self::merge(options, storage_options)),
+ }
+ self
+ }
+
+ pub async fn build(self) -> anyhow::Result<Table> {
+ let hudi_options = self.hudi_options.unwrap_or_default().clone();
+ let mut storage_options = self.storage_options.unwrap_or_default().clone();
+
+ Self::load_storage_options(&mut storage_options);
+
+ let hudi_configs =
+ Self::load_hudi_configs(self.base_uri.clone(), hudi_options, &storage_options)
+ .await
+ .context("Failed to load table properties")?;
+
+ let hudi_configs = Arc::from(hudi_configs);
+ let storage_options = Arc::from(storage_options);
+
+ let timeline = Timeline::new(hudi_configs.clone(), storage_options.clone())
+ .await
+ .context("Failed to load timeline")?;
+
+ let file_system_view = FileSystemView::new(hudi_configs.clone(), storage_options.clone())
+ .await
+ .context("Failed to load file system view")?;
+
+ Ok(Table {
+ hudi_configs,
+ storage_options,
+ timeline,
+ file_system_view,
+ })
+ }
+
+ fn load_storage_options(storage_options: &mut HashMap<String, String>) {
+ Self::imbue_cloud_env_vars(storage_options);
+ }
+
+ async fn load_hudi_configs(
+ base_uri: String,
+ mut hudi_options: HashMap<String, String>,
+ storage_options: &HashMap<String, String>,
+ ) -> anyhow::Result<HudiConfigs> {
+ hudi_options.insert(
+ HudiTableConfig::BasePath.as_ref().to_string(),
+ base_uri.to_string(),
+ );
+
+ // create a [Storage] instance to load properties from storage layer.
+ let storage = Storage::new(
+ Arc::new(storage_options.clone()),
+ Arc::new(HudiConfigs::new(&hudi_options)),
+ )?;
+
+ Self::imbue_table_properties(&mut hudi_options, storage.clone()).await?;
+
+ Self::imbue_global_hudi_configs_if_not_present(&mut hudi_options, storage.clone()).await?;
+
+ let hudi_configs = HudiConfigs::new(hudi_options);
+
+ Table::validate_configs(&hudi_configs).expect("Hudi configs are not valid.");
+ Ok(hudi_configs)
+ }
+
+ fn imbue_cloud_env_vars(options: &mut HashMap<String, String>) {
+ const PREFIXES: [&str; 3] = ["AWS_", "AZURE_", "GOOGLE_"];
+
+ for (key, value) in env::vars() {
+ if PREFIXES.iter().any(|prefix| key.starts_with(prefix))
+ && !options.contains_key(&key.to_ascii_lowercase())
+ {
+ options.insert(key.to_ascii_lowercase(), value);
+ }
+ }
+ }
+
+ async fn imbue_table_properties(
+ options: &mut HashMap<String, String>,
+ storage: Arc<Storage>,
+ ) -> anyhow::Result<()> {
+ let bytes = storage.get_file_data(".hoodie/hoodie.properties").await?;
+ let table_properties = parse_data_for_options(&bytes, "=")?;
+
+ // TODO: handle the case where the same key is present in both table properties and options
Review Comment:
in such case, which option should take precedence?
@xushiyan
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#issuecomment-2408450964
@kazdy i'm cutting release branch within 1 day. do you think you can update this soon?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `TableBuilder` to handle storage_options explicitly [hudi-rs]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#issuecomment-2408827892
## [Codecov](https://app.codecov.io/gh/apache/hudi-rs/pull/163?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `88.60759%` with `9 lines` in your changes missing coverage. Please review.
> Project coverage is 89.68%. Comparing base [(`d5f42f6`)](https://app.codecov.io/gh/apache/hudi-rs/commit/d5f42f68d42cd0469406e3f8307076a6f9a3b49a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e09ab3f`)](https://app.codecov.io/gh/apache/hudi-rs/commit/e09ab3fe2797fdc4c81053b2ad7ef7ed1db0102e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi-rs/pull/163?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [crates/core/src/table/builder.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/163?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL2J1aWxkZXIucnM=) | 88.15% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/163?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #163 +/- ##
==========================================
- Coverage 89.81% 89.68% -0.14%
==========================================
Files 17 18 +1
Lines 884 911 +27
==========================================
+ Hits 794 817 +23
- Misses 90 94 +4
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/hudi-rs/pull/163?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797629237
##########
crates/core/src/table/mod.rs:
##########
@@ -180,91 +169,6 @@ impl Table {
.register_object_store(runtime_env.clone());
}
- async fn load_configs<I, K, V>(
Review Comment:
some of these util functions were updated in main
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
a builder pattern is helpful for users. can you design such that it takes in generic options? we don't have to distinguish hudi option vs storage option from the user side, the build() function should handle it internally.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797675383
##########
python/hudi/_internal.pyi:
##########
@@ -38,7 +38,8 @@ class HudiTable:
def __init__(
self,
table_uri: str,
- options: Optional[Dict[str, str]] = None,
+ hudi_options: Optional[Dict[str, str]] = None,
+ storage_options: Optional[Dict[str, str]] = None,
Review Comment:
will do, I'll introduce HudiTableBuilder in python bindings instead
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797681130
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
@kazdy can you please clarify the benefit of using
```
TableBuilder::with_hudi_options()
TableBuilder::with_storage_options()
```
when there is a generic api to take options and resolve internally, why would you still want to use an explicit api? using a generic api is one less thing caller needs to worry about. From API design pov, it'll be redundant unless being explicit gives additional logic behind 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] introduce TableBuilder to handle storage_options explicitly [hudi-rs]
Posted by "kazdy (via GitHub)" <gi...@apache.org>.
kazdy commented on code in PR #163:
URL: https://github.com/apache/hudi-rs/pull/163#discussion_r1797727245
##########
crates/core/src/table/builder.rs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::config::{HudiConfigs, HUDI_CONF_DIR};
+use crate::storage::utils::{parse_config_data, parse_uri};
+use crate::storage::Storage;
+use crate::table::fs_view::FileSystemView;
+use crate::table::timeline::Timeline;
+use crate::table::Table;
+use anyhow::Context;
+use std::collections::HashMap;
+use std::env;
+use std::path::PathBuf;
+use std::sync::Arc;
+use url::Url;
+
+#[derive(Debug)]
+pub struct TableBuilder {
+ base_url: Url,
+ hudi_options: Option<HashMap<String, String>>,
+ storage_options: Option<HashMap<String, String>>,
+}
+
+impl TableBuilder {
+ pub fn from_uri(uri: &str) -> Self {
+ let base_url = parse_uri(uri).unwrap(); // TODO: handle err
+ TableBuilder {
+ base_url,
+ storage_options: None,
+ hudi_options: None,
+ }
+ }
+ pub fn with_hudi_options(mut self, hudi_options: HashMap<String, String>) -> Self {
+ self.hudi_options = Some(hudi_options);
+ self
+ }
+
+ pub fn with_storage_options(mut self, storage_options: HashMap<String, String>) -> Self {
+ self.storage_options = Some(storage_options);
+ self
+ }
Review Comment:
I think TableBuilder can give more control to the user and be more specific. I'm fine with having a generic API for ease of use, but I still think having this explicit distinction between hudi and storage options is nice and easy to understand when using the API. I was a bit confused at first and wondered if/where can I pass storage options.
Still Table has ::new_with_options() which is generic so that TableBuilder can be more specific?
I agree about validation being done in ::build().
I'm not sure if I'll be able to add more validation now, but I can give it a try in the next PR?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org