You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sdd (via GitHub)" <gi...@apache.org> on 2024/04/04 19:43:32 UTC
[PR] Add Struct Accessors to BoundReferences [iceberg-rust]
sdd opened a new pull request, #317:
URL: https://github.com/apache/iceberg-rust/pull/317
First PR to come out of breaking up https://github.com/apache/iceberg-rust/pull/241.
Adds `StructAccessor`, which is added to `BoundReference` as a means of retrieving a field's value from a (possibly nested) Struct.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554540831
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,119 @@
+// 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::{Datum, Literal, PrimitiveType, Struct};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Box<StructAccessor>),
+ Type(PrimitiveType),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: usize,
+ r#type: PrimitiveType,
+ inner: Option<Box<StructAccessor>>,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: usize, r#type: PrimitiveType) -> Self {
+ StructAccessor {
+ position,
+ r#type,
+ inner: None,
+ }
+ }
+
+ pub(crate) fn wrap(position: usize, inner: Box<StructAccessor>) -> Self {
+ StructAccessor {
+ position,
+ r#type: inner.r#type().clone(),
+ inner: Some(inner),
+ }
+ }
+
+ pub(crate) fn position(&self) -> usize {
+ self.position
+ }
+
+ pub(crate) fn r#type(&self) -> &PrimitiveType {
+ &self.r#type
+ }
+
+ pub(crate) fn get<'a>(&'a self, container: &'a Struct) -> Datum {
+ match &self.inner {
+ None => {
+ let Literal::Primitive(literal) = &container[self.position] else {
+ panic!("Expected Literal to be Primitive");
Review Comment:
Sure, that's much nicer. Done.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554175122
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: i32, r#type: Type) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Type(r#type),
+ }
+ }
+
+ pub(crate) fn wrap(position: i32, inner: StructAccessorRef) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Inner(inner),
+ }
+ }
+
+ pub fn position(&self) -> i32 {
+ self.position
+ }
+
+ fn r#type(&self) -> &Type {
+ match &self.inner_or_type {
+ InnerOrType::Inner(inner) => inner.r#type(),
+ InnerOrType::Type(r#type) => r#type,
+ }
+ }
+
+ fn get<'a>(&'a self, container: &'a Struct) -> &Literal {
Review Comment:
Ok, will update
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -1141,6 +1141,11 @@ impl Struct {
},
)
}
+
+ /// Gets a ref to the field at `position` within the `Struct`
+ pub fn get(&self, position: i32) -> &Literal {
Review Comment:
Good idea! Will do
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554161905
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
Review Comment:
Sure, will change. I saw we'd used `i32` almost everywhere else so went with that but `usize` works better
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554540767
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,119 @@
+// 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::{Datum, Literal, PrimitiveType, Struct};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
Review Comment:
Whoops! Done.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#issuecomment-2038350682
PTAL @liurenjie1024 and @marvinlanhenke - extracted from https://github.com/apache/iceberg-rust/pull/241 and added 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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554174651
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
Review Comment:
Wil do!
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
Review Comment:
Will do!
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1553151792
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
Review Comment:
This seems inefficient to me, each time we call `r#type`, it needs to recursively call inners for nested field. How about just following:
```rust
r#type: Type,
inner: Option<StructAccessorRef>
```
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
How about using SchemaVisitor to build this?
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
Review Comment:
How about just name it `Accessor`? This is in fact field accessor.
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
Review Comment:
```suggestion
Inner(Box<StructAccessor>),
```
I think the inner accessor is not shared, so it would be more efficient to use `Box`
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
Also it would be better to add some uts to show the result.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -1141,6 +1141,11 @@ impl Struct {
},
)
}
+
+ /// Gets a ref to the field at `position` within the `Struct`
+ pub fn get(&self, position: i32) -> &Literal {
Review Comment:
How about implementing `Index` trait?
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: i32, r#type: Type) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Type(r#type),
+ }
+ }
+
+ pub(crate) fn wrap(position: i32, inner: StructAccessorRef) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Inner(inner),
+ }
+ }
+
+ pub fn position(&self) -> i32 {
+ self.position
+ }
+
+ fn r#type(&self) -> &Type {
+ match &self.inner_or_type {
+ InnerOrType::Inner(inner) => inner.r#type(),
+ InnerOrType::Type(r#type) => r#type,
+ }
+ }
+
+ fn get<'a>(&'a self, container: &'a Struct) -> &Literal {
Review Comment:
Should we return `Datum` here? This better follows python/java implementation.
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: i32, r#type: Type) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Type(r#type),
+ }
+ }
+
+ pub(crate) fn wrap(position: i32, inner: StructAccessorRef) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Inner(inner),
+ }
+ }
+
+ pub fn position(&self) -> i32 {
+ self.position
+ }
+
+ fn r#type(&self) -> &Type {
+ match &self.inner_or_type {
+ InnerOrType::Inner(inner) => inner.r#type(),
+ InnerOrType::Type(r#type) => r#type,
+ }
+ }
+
+ fn get<'a>(&'a self, container: &'a Struct) -> &Literal {
Review Comment:
I think we should return error here, the input may come user.
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
Review Comment:
How about changing this to `usize`? It's somehow like the position index of an array.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554171392
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
Review Comment:
This was deliberate - in the Java implementation, `Accessor` is an interface and generic over `T`, with T usually being `StructLike`, and `StructLike` is itself an interface. Since we only have `Struct` at the moment , rather than several structs that implement a `StructLike` trait, I went for `StructAccessor`, since ours is closer to `Accessor<Struct>` in the Java implementation, and this allows us to implement a more generic `Accessor` at some point in the future alongside `StructAccessor`.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554425354
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
`SchemaVisitor` won't work for this without some modifications. We need to know the index within `Schema.fields` for each field to create its accessor, and `visit_struct` does not provide this at the moment.
`visit_struct` and `SchemaVisitor` could be modified to pass the field index to the visitor's methods. Can we keep this PR as-is and have a TODO so that we can move on with this working as-is and I or someone else can pick up this refactor in another 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: 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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554520971
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,119 @@
+// 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::{Datum, Literal, PrimitiveType, Struct};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Box<StructAccessor>),
+ Type(PrimitiveType),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: usize,
+ r#type: PrimitiveType,
+ inner: Option<Box<StructAccessor>>,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: usize, r#type: PrimitiveType) -> Self {
+ StructAccessor {
+ position,
+ r#type,
+ inner: None,
+ }
+ }
+
+ pub(crate) fn wrap(position: usize, inner: Box<StructAccessor>) -> Self {
+ StructAccessor {
+ position,
+ r#type: inner.r#type().clone(),
+ inner: Some(inner),
+ }
+ }
+
+ pub(crate) fn position(&self) -> usize {
+ self.position
+ }
+
+ pub(crate) fn r#type(&self) -> &PrimitiveType {
+ &self.r#type
+ }
+
+ pub(crate) fn get<'a>(&'a self, container: &'a Struct) -> Datum {
+ match &self.inner {
+ None => {
+ let Literal::Primitive(literal) = &container[self.position] else {
+ panic!("Expected Literal to be Primitive");
+ };
+
+ Datum::new(self.r#type().clone(), literal.clone())
+ }
+ Some(inner) => {
+ let Literal::Struct(wrapped) = &container[self.position] else {
+ panic!("Nested accessor should only be wrapping a Struct");
Review Comment:
Ditto.
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
> Will do, re SchemaVisitor. Sorry Renjie, not sure what you mean with the second comment though?
Sorry, I mean adding some unit test and example code in api to demonstrate its usage, like what python does here: https://github.com/apache/iceberg-python/blob/d3db8401cd006fcbacf1d9a2502248ce4228ff06/pyiceberg/schema.py#L1162
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,119 @@
+// 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::{Datum, Literal, PrimitiveType, Struct};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
Review Comment:
Should we remove this?
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
Review Comment:
This is `WrappedPositionAccessor` in java, and it's only used to access struct. Sounds reasonable to me, thanks for explaination.
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
> SchemaVisitor won't work for this without some modifications. We need to know the index within Schema.fields for each field to create its accessor, and visit_struct does not provide this at the moment.
Sorry, I don't get your point, following code would work?
```rust
fn visit_struct(&self, struct: &Struct, field_results: Vec<Option<StructAccessor>>) {
struct.fields.zip(field_result).enumerate().map(|(index, (field, field_result)|) {}
}
```
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,119 @@
+// 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::{Datum, Literal, PrimitiveType, Struct};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Box<StructAccessor>),
+ Type(PrimitiveType),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: usize,
+ r#type: PrimitiveType,
+ inner: Option<Box<StructAccessor>>,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: usize, r#type: PrimitiveType) -> Self {
+ StructAccessor {
+ position,
+ r#type,
+ inner: None,
+ }
+ }
+
+ pub(crate) fn wrap(position: usize, inner: Box<StructAccessor>) -> Self {
+ StructAccessor {
+ position,
+ r#type: inner.r#type().clone(),
+ inner: Some(inner),
+ }
+ }
+
+ pub(crate) fn position(&self) -> usize {
+ self.position
+ }
+
+ pub(crate) fn r#type(&self) -> &PrimitiveType {
+ &self.r#type
+ }
+
+ pub(crate) fn get<'a>(&'a self, container: &'a Struct) -> Datum {
+ match &self.inner {
+ None => {
+ let Literal::Primitive(literal) = &container[self.position] else {
+ panic!("Expected Literal to be Primitive");
Review Comment:
I think we should return error rather than panic here? The invalid input may come from malformed file, and it should not crash the program.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554560769
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
Sorry for misclarification, in fact I mean the `r#struct` method in `SchemaVisitor`: https://github.com/apache/iceberg-rust/blob/1c2a20b13e67e4f91a44d49fa1b0e3432cd48432/crates/iceberg/src/spec/schema.rs#L324
But I agree that it could be left to do it later.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1553285045
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
+}
+
+pub(crate) type StructAccessorRef = Arc<StructAccessor>;
+
+impl StructAccessor {
+ pub(crate) fn new(position: i32, r#type: Type) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Type(r#type),
+ }
+ }
+
+ pub(crate) fn wrap(position: i32, inner: StructAccessorRef) -> Self {
+ StructAccessor {
+ position,
+ inner_or_type: InnerOrType::Inner(inner),
+ }
+ }
+
+ pub fn position(&self) -> i32 {
+ self.position
+ }
+
+ fn r#type(&self) -> &Type {
+ match &self.inner_or_type {
+ InnerOrType::Inner(inner) => inner.r#type(),
+ InnerOrType::Type(r#type) => r#type,
+ }
+ }
+
+ fn get<'a>(&'a self, container: &'a Struct) -> &Literal {
Review Comment:
> Should we return `Datum` here? This better follows python/java implementation.
unless we explicitly need `Literal` somewhere downstream (haven't checked python/java yet). I'd definitely +1 for returning `Datum` here, which should be easier to handle downstream.
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554176015
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
Will do, re SchemaVisitor. Sorry Renjie, not sure what you mean with the second comment though?
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554542369
##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -137,9 +142,55 @@ impl SchemaBuilder {
name_to_id,
lowercase_name_to_id,
id_to_name,
+
+ field_id_to_accessor,
})
}
+ fn build_accessors(&self) -> HashMap<i32, Arc<StructAccessor>> {
Review Comment:
Sorry Renjie, I still don't follow! `visit_struct` already exists and has a different signature to that?
https://github.com/apache/iceberg-rust/blob/4e89ac71c3ea77b9270b71dac52b5c3e50c36526/crates/iceberg/src/spec/schema.rs#L365
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#discussion_r1554163204
##########
crates/iceberg/src/expr/accessor.rs:
##########
@@ -0,0 +1,114 @@
+// 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::{Literal, Struct, Type};
+use serde_derive::{Deserialize, Serialize};
+use std::sync::Arc;
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+enum InnerOrType {
+ Inner(Arc<StructAccessor>),
+ Type(Type),
+}
+
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub struct StructAccessor {
+ position: i32,
+ inner_or_type: InnerOrType,
Review Comment:
That duplicates the type at each level, but you have a point, we're optimising for time efficiency rather than space. Will 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: 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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317#issuecomment-2041050879
@liurenjie1024 I've added a test as well now for build_accessors :-)
--
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] Add Struct Accessors to BoundReferences [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #317:
URL: https://github.com/apache/iceberg-rust/pull/317
--
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