You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ZENOTME (via GitHub)" <gi...@apache.org> on 2024/05/29 15:40:34 UTC
[PR] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
ZENOTME opened a new pull request, #386:
URL: https://github.com/apache/iceberg-rust/pull/386
This PR fix #378.
1. It makes PrimitiveLiteral and Literal not be PartialOrd, Ord. And only Datum PartialOrd.
2. After 1, it also changes the Manifest and ManifestList to use Datum in `lower_bound` && `upper_bound`
After Literal is not Ord, we can't use `BTreeMap<Literal,_>` anymore. I use `HashMap` to replace it. But to make `HashMap` hashable, we need to assign an order for it. I implement this in `Map`.
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386#discussion_r1621824738
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -713,8 +866,117 @@ impl Datum {
}
}
+/// Map is a collection of key-value pairs with a key type and a value type.
+/// It used in Literal::Map, to make it hashable, the order of key-value pairs is stored in a separate vector
+/// so that we can hash the map in a deterministic way. But it also means that the order of key-value pairs is matter
+/// for the hash value.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct Map {
+ index: HashMap<Literal, usize>,
+ pair: Vec<(Literal, Option<Literal>)>,
+}
+
+impl Map {
+ /// Creates a new empty map.
+ pub fn new() -> Self {
+ Self {
+ index: HashMap::new(),
+ pair: Vec::new(),
+ }
+ }
+
+ /// Return the number of key-value pairs in the map.
+ pub fn len(&self) -> usize {
+ self.pair.len()
+ }
+
+ /// Returns true if the map contains no elements.
+ pub fn is_empty(&self) -> bool {
+ self.pair.is_empty()
+ }
+
+ /// Inserts a key-value pair into the map.
+ /// If the map did not have this key present, None is returned.
+ /// If the map did have this key present, the value is updated, and the old value is returned.
+ pub fn insert(&mut self, key: Literal, value: Option<Literal>) -> Option<Option<Literal>> {
+ if let Some(index) = self.index.get(&key) {
+ let old_value = std::mem::replace(&mut self.pair[*index].1, value);
+ Some(old_value)
+ } else {
+ self.pair.push((key.clone(), value));
+ self.index.insert(key, self.pair.len() - 1);
+ None
+ }
+ }
+
+ /// Returns a reference to the value corresponding to the key.
+ /// If the key is not present in the map, None is returned.
+ pub fn get(&self, key: &Literal) -> Option<&Option<Literal>> {
+ self.index.get(key).map(|index| &self.pair[*index].1)
+ }
+
+ /// The order of map is matter, so this method used to compare two maps has same key-value pairs without considering the order.
+ pub fn has_same_content(&self, other: &Map) -> bool {
+ if self.len() != other.len() {
+ return false;
+ }
+
+ for (key, value) in &self.pair {
+ if let Some(other_value) = other.get(key) {
+ if value != other_value {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
Review Comment:
minor nit here:
```suggestion
for (key, value) in &self.pair {
match other.get(key) {
Some(other_value) if value == other_value => (),
_ => return false,
}
}
```
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386#discussion_r1620852535
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -118,6 +118,15 @@ impl Type {
matches!(self, Type::Struct(_) | Type::List(_) | Type::Map(_))
}
+ /// Convert Type to reference of PrimitiveType
+ pub fn as_primitive_type_ref(&self) -> Option<&PrimitiveType> {
Review Comment:
LGTM. This is to align with the following `pub fn as_struct_type(self) -> Option<StructType>`. We need a new name if consider `as_` returns a ref, e.g. `pub fn as_struct_type_owned(self) -> Option<StructType>`.
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386#issuecomment-2137737297
cc @liurenjie1024 @Xuanwo @Fokko @marvinlanhenke
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386#discussion_r1620764612
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -118,6 +118,15 @@ impl Type {
matches!(self, Type::Struct(_) | Type::List(_) | Type::Map(_))
}
+ /// Convert Type to reference of PrimitiveType
+ pub fn as_primitive_type_ref(&self) -> Option<&PrimitiveType> {
Review Comment:
```suggestion
pub fn as_primitive_type(&self) -> Option<&PrimitiveType> {
```
I think in rust `as_` usually returns a ref, so this would be enough?
--
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] fix: make PrimitiveLiteral and Literal not be Ord [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #386:
URL: https://github.com/apache/iceberg-rust/pull/386#discussion_r1621647274
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -118,6 +118,15 @@ impl Type {
matches!(self, Type::Struct(_) | Type::List(_) | Type::Map(_))
}
+ /// Convert Type to reference of PrimitiveType
+ pub fn as_primitive_type_ref(&self) -> Option<&PrimitiveType> {
Review Comment:
Or `to_struct_type` maybe more rusty?
--
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