You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jonathanc-n (via GitHub)" <gi...@apache.org> on 2025/05/07 18:44:51 UTC
[PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
jonathanc-n opened a new pull request, #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Build on https://github.com/apache/iceberg-rust/pull/1116
## What changes are included in this PR?
Adds IndexByName and index by id to name mapping
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
## Are these changes tested?
Yes, unit tests
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n closed pull request #1299: feat: Add `IndexByName` and `IndexById` to Namemapping
URL: https://github.com/apache/iceberg-rust/pull/1299
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2094956624
##########
crates/iceberg/src/spec/name_mapping/visitor.rs:
##########
@@ -0,0 +1,30 @@
+// 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::spec::MappedField;
+
+/// A trait for visiting and transforming a name mapping
+pub trait NameMappingVisitor {
+ /// Aggregated result of `MappedField`s
+ type S;
+
+ /// Handles entire `NameMapping` field
+ fn mapping(&self, field_result: Self::S) -> Self::S;
+
+ /// Handles a single `MappedField`
+ fn field(&self, field: &MappedField, field_result: Self::S) -> Self::S;
+}
Review Comment:
```suggestion
pub trait NameMappingVisitor {
type T;
/// Aggregated result of `MappedField`s
type S;
/// Handles entire `NameMapping` field
fn mapping(&mut self, mapping: &NameMapping, field_result: Vec<Self::T>) -> Result<Self::S>;
/// Handles a single `MappedField`
fn field(&mut self, field: &MappedField, field_result: Vec<Self::T>) -> Result<Self::T>;
}
```
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -29,19 +40,59 @@ pub const DEFAULT_SCHEMA_NAME_MAPPING: &str = "schema.name-mapping.default";
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[serde(transparent)]
pub struct NameMapping {
- root: Vec<MappedField>,
+ // this is the one field that actually gets round‑tripped
+ root: Vec<Arc<MappedField>>,
+ // both of these get skipped during (de)serialization and default to empty
+ #[serde(skip)]
+ id_to_field: HashMap<i32, Arc<MappedField>>,
+ #[serde(skip)]
+ name_to_field: HashMap<String, Arc<MappedField>>,
}
impl NameMapping {
- /// Create a new [`NameMapping`] given a collection of mapped fields.
- pub fn new(fields: Vec<MappedField>) -> Self {
- Self { root: fields }
+ /// Creates new `NameMapping` instance
+ pub fn new(schema: &Schema) -> Self {
+ // Create `MappedField` instances by visiting schema.
+ let root = visit_schema(schema, &mut CreateMapping).unwrap();
Review Comment:
We can construct a `NameMapping` without index here, and then assign the indcies to it before returning.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -77,16 +128,170 @@ impl MappedField {
pub fn names(&self) -> &[String] {
&self.names
}
+}
- /// Get a reference to the field mapping for any child fields.
- pub fn fields(&self) -> &[Arc<MappedField>] {
- &self.fields
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(name_mapping: &Vec<Arc<MappedField>>, visitor: &V) -> Result<V::S>
+where
+ V: NameMappingVisitor,
+ V::S: IntoIterator + FromIterator<<V::S as IntoIterator>::Item>,
+{
+ let root_result = visit_fields(name_mapping, visitor);
+ Ok(visitor.mapping(root_result))
+}
+
+/// Recursively visits a slice of mapped fields using visitor
+fn visit_fields<V>(fields: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where
+ V: NameMappingVisitor,
+ V::S: IntoIterator + FromIterator<<V::S as IntoIterator>::Item>,
Review Comment:
Please refine the visiting logic with suggested visitor.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2086378297
##########
crates/iceberg/src/expr/visitors/name_mapping_visitor.rs:
##########
Review Comment:
Sorry for misclarification, I mean we should move this to `spec/name_maping/visitor.rs`. It's odd to put `NameMappingVisitor` in expr module.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let root_result = visit_fields(namespace, visitor);
+ visitor.mapping(root_result)
+}
+
+/// Recursively visits a slice of mapped fields using visitor
+fn visit_fields<V>(fields: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let mut results: Vec<V::T> = Vec::new();
+
+ for field in fields {
+ let child_result = visit_fields(&field.fields, visitor);
+ let field_result = visitor.field(field, child_result);
+ results.push(field_result);
+ }
+
+ visitor.fields(results)
+}
+
+struct IndexByName {}
+
+impl NameMappingVisitor for IndexByName {
+ type S = HashMap<String, Arc<MappedField>>;
+ type T = HashMap<String, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<String, Arc<MappedField>>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ child_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ let mut result = child_result;
+
+ for name in &field.names {
+ for (child_key, child_field) in result.clone().iter() {
+ let composite_key = format!("{}.{}", name, child_key);
+ result.insert(composite_key, child_field.clone());
+ }
+ }
+
+ for name in &field.names {
+ result.insert(name.clone(), Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct IndexById {}
+
+impl NameMappingVisitor for IndexById {
+ type S = HashMap<i32, Arc<MappedField>>;
+ type T = HashMap<i32, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<i32, Arc<MappedField>>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ field_results: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ let mut result = field_results;
+
+ if let Some(id) = field.field_id {
+ result.insert(id, Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct CreateMapping;
+
+impl SchemaVisitor for CreateMapping {
+ type T = Vec<Arc<MappedField>>;
+
+ fn schema(&mut self, _schema: &Schema, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn field(&mut self, _field: &NestedFieldRef, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn r#struct(&mut self, struct_type: &StructType, results: Vec<Self::T>) -> Result<Self::T> {
+ let mapped_fields = struct_type
+ .fields()
+ .iter()
+ .zip(results)
+ .map(|(field, result)| {
+ Arc::new(MappedField::new(
+ Some(field.id),
+ vec![field.name.clone()],
+ result,
+ ))
+ })
+ .collect::<Vec<Arc<MappedField>>>();
+
+ Ok(mapped_fields)
+ }
+
+ fn list(&mut self, list: &ListType, value: Self::T) -> Result<Self::T> {
+ Ok(vec![Arc::new(MappedField::new(
+ Some(list.element_field.id),
+ vec!["element".to_string()],
+ value,
+ ))])
+ }
+
+ fn map(&mut self, map: &MapType, key_value: Self::T, value: Self::T) -> Result<Self::T> {
+ Ok(vec![
+ Arc::new(MappedField::new(
+ Some(map.key_field.id),
+ vec!["key".to_string()],
+ key_value,
+ )),
+ Arc::new(MappedField::new(
+ Some(map.value_field.id),
+ vec!["value".to_string()],
Review Comment:
See https://github.com/apache/iceberg-rust/blob/7eb00d5e1571b77c35b5433ac6959ee7340b46ec/crates/iceberg/src/spec/datatypes.rs#L43
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
Review Comment:
```suggestion
fn visit_name_mapping<V>(namespace: &NameMapping, visitor: &V) -> V::S
```
It's odd that the name is `visit_name_mapping` while accepts a list of fields.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let root_result = visit_fields(namespace, visitor);
+ visitor.mapping(root_result)
+}
+
+/// Recursively visits a slice of mapped fields using visitor
+fn visit_fields<V>(fields: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let mut results: Vec<V::T> = Vec::new();
+
+ for field in fields {
+ let child_result = visit_fields(&field.fields, visitor);
+ let field_result = visitor.field(field, child_result);
+ results.push(field_result);
+ }
+
+ visitor.fields(results)
+}
+
+struct IndexByName {}
+
+impl NameMappingVisitor for IndexByName {
+ type S = HashMap<String, Arc<MappedField>>;
+ type T = HashMap<String, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<String, Arc<MappedField>>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ child_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ let mut result = child_result;
+
+ for name in &field.names {
+ for (child_key, child_field) in result.clone().iter() {
+ let composite_key = format!("{}.{}", name, child_key);
+ result.insert(composite_key, child_field.clone());
+ }
+ }
+
+ for name in &field.names {
+ result.insert(name.clone(), Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct IndexById {}
+
+impl NameMappingVisitor for IndexById {
+ type S = HashMap<i32, Arc<MappedField>>;
+ type T = HashMap<i32, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<i32, Arc<MappedField>>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ field_results: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ let mut result = field_results;
+
+ if let Some(id) = field.field_id {
+ result.insert(id, Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct CreateMapping;
+
+impl SchemaVisitor for CreateMapping {
+ type T = Vec<Arc<MappedField>>;
+
+ fn schema(&mut self, _schema: &Schema, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn field(&mut self, _field: &NestedFieldRef, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn r#struct(&mut self, struct_type: &StructType, results: Vec<Self::T>) -> Result<Self::T> {
+ let mapped_fields = struct_type
+ .fields()
+ .iter()
+ .zip(results)
+ .map(|(field, result)| {
+ Arc::new(MappedField::new(
+ Some(field.id),
+ vec![field.name.clone()],
+ result,
+ ))
+ })
+ .collect::<Vec<Arc<MappedField>>>();
+
+ Ok(mapped_fields)
+ }
+
+ fn list(&mut self, list: &ListType, value: Self::T) -> Result<Self::T> {
+ Ok(vec![Arc::new(MappedField::new(
+ Some(list.element_field.id),
+ vec!["element".to_string()],
Review Comment:
Please use constant here: https://github.com/apache/iceberg-rust/blob/7eb00d5e1571b77c35b5433ac6959ee7340b46ec/crates/iceberg/src/spec/datatypes.rs#L39
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let root_result = visit_fields(namespace, visitor);
+ visitor.mapping(root_result)
+}
+
+/// Recursively visits a slice of mapped fields using visitor
+fn visit_fields<V>(fields: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
+where V: NameMappingVisitor {
+ let mut results: Vec<V::T> = Vec::new();
+
+ for field in fields {
+ let child_result = visit_fields(&field.fields, visitor);
+ let field_result = visitor.field(field, child_result);
+ results.push(field_result);
+ }
+
+ visitor.fields(results)
+}
+
+struct IndexByName {}
+
+impl NameMappingVisitor for IndexByName {
+ type S = HashMap<String, Arc<MappedField>>;
+ type T = HashMap<String, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<String, Arc<MappedField>>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ child_result: HashMap<String, Arc<MappedField>>,
+ ) -> HashMap<String, Arc<MappedField>> {
+ let mut result = child_result;
+
+ for name in &field.names {
+ for (child_key, child_field) in result.clone().iter() {
+ let composite_key = format!("{}.{}", name, child_key);
+ result.insert(composite_key, child_field.clone());
+ }
+ }
+
+ for name in &field.names {
+ result.insert(name.clone(), Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct IndexById {}
+
+impl NameMappingVisitor for IndexById {
+ type S = HashMap<i32, Arc<MappedField>>;
+ type T = HashMap<i32, Arc<MappedField>>;
+
+ fn mapping(
+ &self,
+ field_result: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_result
+ }
+
+ fn fields(
+ &self,
+ field_results: Vec<HashMap<i32, Arc<MappedField>>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ field_results
+ .into_iter()
+ .fold(HashMap::new(), |mut acc, map| {
+ acc.extend(map);
+ acc
+ })
+ }
+
+ fn field(
+ &self,
+ field: &MappedField,
+ field_results: HashMap<i32, Arc<MappedField>>,
+ ) -> HashMap<i32, Arc<MappedField>> {
+ let mut result = field_results;
+
+ if let Some(id) = field.field_id {
+ result.insert(id, Arc::new(field.clone()));
+ }
+ result
+ }
+}
+
+struct CreateMapping;
+
+impl SchemaVisitor for CreateMapping {
+ type T = Vec<Arc<MappedField>>;
+
+ fn schema(&mut self, _schema: &Schema, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn field(&mut self, _field: &NestedFieldRef, value: Self::T) -> Result<Self::T> {
+ Ok(value)
+ }
+
+ fn r#struct(&mut self, struct_type: &StructType, results: Vec<Self::T>) -> Result<Self::T> {
+ let mapped_fields = struct_type
+ .fields()
+ .iter()
+ .zip(results)
+ .map(|(field, result)| {
+ Arc::new(MappedField::new(
+ Some(field.id),
+ vec![field.name.clone()],
+ result,
+ ))
+ })
+ .collect::<Vec<Arc<MappedField>>>();
+
+ Ok(mapped_fields)
+ }
+
+ fn list(&mut self, list: &ListType, value: Self::T) -> Result<Self::T> {
+ Ok(vec![Arc::new(MappedField::new(
+ Some(list.element_field.id),
+ vec!["element".to_string()],
+ value,
+ ))])
+ }
+
+ fn map(&mut self, map: &MapType, key_value: Self::T, value: Self::T) -> Result<Self::T> {
+ Ok(vec![
+ Arc::new(MappedField::new(
+ Some(map.key_field.id),
+ vec!["key".to_string()],
Review Comment:
See: https://github.com/apache/iceberg-rust/blob/7eb00d5e1571b77c35b5433ac6959ee7340b46ec/crates/iceberg/src/spec/datatypes.rs#L41
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
Review Comment:
Also the return value should be a `Result`
##########
crates/iceberg/src/expr/visitors/name_mapping_visitor.rs:
##########
@@ -0,0 +1,35 @@
+// 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::spec::MappedField;
+
+/// A trait for visiting and transforming a name mapping
+pub trait NameMappingVisitor {
+ /// Aggregated result of `MappedField`s
+ type S;
+ /// Result type for processing one `MappedField`
+ type T;
+
+ /// Handles entire `NameMapping` field
+ fn mapping(&self, field_result: Self::S) -> Self::S;
+
+ /// Takes all visited child maps and merges into one map
+ fn fields(&self, field_results: Vec<Self::T>) -> Self::S;
Review Comment:
I don't think we need this method. It's required in java since they have a middle layer like `MappedFields`, but this is not the case for rust. I think the whole visitor trait could be simplified as following:
```rust
trait Visitor {
type S;
type T;
fn mapping(&self, name_mapping: &NameMaping, children: Vec<Self::T>) -> Result<Self::S>;
fn field(&self, field: &MappedField, children: Vec<Self::T>) -> Result<Self::T>;
}
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2093873639
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
Review Comment:
@liurenjie1024 This should be good for another look, thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2081331067
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -17,31 +17,72 @@
//! Iceberg name mapping.
-use std::sync::Arc;
+use std::collections::HashMap;
use serde::{Deserialize, Serialize};
use serde_with::{serde_as, DefaultOnNull};
+use crate::spec::{
+ visit_schema, ListType, MapType, NestedFieldRef, PrimitiveType, Schema, SchemaVisitor,
+ StructType,
+};
+use crate::Result;
+
/// Property name for name mapping.
pub const DEFAULT_SCHEMA_NAME_MAPPING: &str = "schema.name-mapping.default";
/// Iceberg fallback field name to ID mapping.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
-#[serde(transparent)]
pub struct NameMapping {
root: Vec<MappedField>,
+ id_to_field: HashMap<i32, MappedField>,
+ name_to_field: HashMap<String, MappedField>,
}
impl NameMapping {
- /// Create a new [`NameMapping`] given a collection of mapped fields.
- pub fn new(fields: Vec<MappedField>) -> Self {
- Self { root: fields }
+ /// Creates new `NameMapping` instance
+ pub fn new(
+ root: Vec<MappedField>,
+ id_to_field: HashMap<i32, MappedField>,
+ name_to_field: HashMap<String, MappedField>,
Review Comment:
These should be constructed in constructor, not provided by user.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -55,7 +96,7 @@ pub struct MappedField {
#[serde(default)]
#[serde(skip_serializing_if = "Vec::is_empty")]
#[serde_as(deserialize_as = "DefaultOnNull")]
- fields: Vec<Arc<MappedField>>,
+ fields: Vec<MappedField>,
Review Comment:
Yes, clone would be quite expensive for `MappedField`.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -179,44 +393,48 @@ mod tests {
#[test]
fn test_json_name_mapping_deserialization() {
let name_mapping = r#"
- [
{
- "field-id": 1,
- "names": [
- "id",
- "record_id"
- ]
- },
- {
- "field-id": 2,
- "names": [
- "data"
- ]
- },
- {
- "field-id": 3,
- "names": [
- "location"
- ],
- "fields": [
+ "root": [
Review Comment:
Yes, we can't change json here, it's required by iceberg spec.
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -79,15 +120,188 @@ impl MappedField {
}
/// Get a reference to the field mapping for any child fields.
- pub fn fields(&self) -> &[Arc<MappedField>] {
+ pub fn fields(&self) -> &[MappedField] {
&self.fields
}
}
+/// A trait for visiting and transforming a name mapping
+trait NameMappingVisitor {
Review Comment:
Moving visitors to `visitor` module?
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -17,31 +17,72 @@
//! Iceberg name mapping.
-use std::sync::Arc;
+use std::collections::HashMap;
use serde::{Deserialize, Serialize};
use serde_with::{serde_as, DefaultOnNull};
+use crate::spec::{
+ visit_schema, ListType, MapType, NestedFieldRef, PrimitiveType, Schema, SchemaVisitor,
+ StructType,
+};
+use crate::Result;
+
/// Property name for name mapping.
pub const DEFAULT_SCHEMA_NAME_MAPPING: &str = "schema.name-mapping.default";
/// Iceberg fallback field name to ID mapping.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
-#[serde(transparent)]
pub struct NameMapping {
root: Vec<MappedField>,
+ id_to_field: HashMap<i32, MappedField>,
+ name_to_field: HashMap<String, MappedField>,
}
impl NameMapping {
- /// Create a new [`NameMapping`] given a collection of mapped fields.
- pub fn new(fields: Vec<MappedField>) -> Self {
- Self { root: fields }
+ /// Creates new `NameMapping` instance
+ pub fn new(
+ root: Vec<MappedField>,
+ id_to_field: HashMap<i32, MappedField>,
+ name_to_field: HashMap<String, MappedField>,
+ ) -> Self {
+ Self {
+ root,
+ id_to_field,
+ name_to_field,
+ }
}
/// Get a reference to fields which are to be mapped from name to field ID.
pub fn fields(&self) -> &[MappedField] {
&self.root
}
+
+ /// Parses name_mapping from JSON.
+ pub fn parse_name_mapping(name_mapping: &str) -> Result<Self> {
Review Comment:
nit: Implement `FromStr` here?
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -79,15 +120,188 @@ impl MappedField {
}
/// Get a reference to the field mapping for any child fields.
- pub fn fields(&self) -> &[Arc<MappedField>] {
+ pub fn fields(&self) -> &[MappedField] {
&self.fields
}
}
+/// A trait for visiting and transforming a name mapping
+trait NameMappingVisitor {
+ /// Aggregated result of `MappedField`s
+ type S;
+ /// Result type for processing one `MappedField`
+ type T;
+
+ /// Handles entire `NameMapping` field
+ fn mapping(&self, field_result: Self::S) -> Self::S;
+
+ /// Handles accessing multiple `MappedField`
+ fn fields(&self, field_results: Vec<Self::T>) -> Self::S;
Review Comment:
This is incorrect, we need to pass `&[MappedField]` here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2082404424
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -79,15 +120,188 @@ impl MappedField {
}
/// Get a reference to the field mapping for any child fields.
- pub fn fields(&self) -> &[Arc<MappedField>] {
+ pub fn fields(&self) -> &[MappedField] {
&self.fields
}
}
+/// A trait for visiting and transforming a name mapping
+trait NameMappingVisitor {
+ /// Aggregated result of `MappedField`s
+ type S;
+ /// Result type for processing one `MappedField`
+ type T;
+
+ /// Handles entire `NameMapping` field
+ fn mapping(&self, field_result: Self::S) -> Self::S;
+
+ /// Handles accessing multiple `MappedField`
+ fn fields(&self, field_results: Vec<Self::T>) -> Self::S;
Review Comment:
Sorry, what the function does is misleading. the fields function takes all the field maps from that level and combines them into one map. I will change the comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2079179261
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -55,7 +96,7 @@ pub struct MappedField {
#[serde(default)]
#[serde(skip_serializing_if = "Vec::is_empty")]
#[serde_as(deserialize_as = "DefaultOnNull")]
- fields: Vec<Arc<MappedField>>,
+ fields: Vec<MappedField>,
Review Comment:
The reasoning for this was for being cheaply cloneable from what I understood, since this can be a heavily nested field.
Original thread is [here](https://github.com/apache/iceberg-rust/pull/1116#discussion_r2046801281).
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -179,44 +393,48 @@ mod tests {
#[test]
fn test_json_name_mapping_deserialization() {
let name_mapping = r#"
- [
{
- "field-id": 1,
- "names": [
- "id",
- "record_id"
- ]
- },
- {
- "field-id": 2,
- "names": [
- "data"
- ]
- },
- {
- "field-id": 3,
- "names": [
- "location"
- ],
- "fields": [
+ "root": [
Review Comment:
From what I understood, this JSON shouldn't change (`root` field and inclusion of the mappings below), which can be achieved using the `serde(skip)` derive too.
Although, I'll defer to someone more with a lot more experience of the iceberg project too here :smile:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2087291885
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -84,9 +133,187 @@ impl MappedField {
}
}
+/// Recursively visits the entire name mapping using visitor
+fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S
Review Comment:
This is because when creating a new `NameMapping` we call `visit_schema` which creates an array of mapped fields which are then called on by `index_by_id` + `index_by_name`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#discussion_r2078283965
##########
crates/iceberg/src/spec/name_mapping/mod.rs:
##########
@@ -55,7 +96,7 @@ pub struct MappedField {
#[serde(default)]
#[serde(skip_serializing_if = "Vec::is_empty")]
#[serde_as(deserialize_as = "DefaultOnNull")]
- fields: Vec<Arc<MappedField>>,
+ fields: Vec<MappedField>,
Review Comment:
No need for arc here, the mapping is created once + there won't be a time where more than one thread is accessing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#issuecomment-2859866566
cc @jdockerty @liurenjie1024
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Add `IndexByName` and `IndexById` to Namemapping [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #1299:
URL: https://github.com/apache/iceberg-rust/pull/1299#issuecomment-2867738652
@jdockerty @liurenjie1024 Thanks for the reviews. Should be fixed now!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org