You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "DerGut (via GitHub)" <gi...@apache.org> on 2025/09/12 13:40:21 UTC
[PR] Derive PartialOrd [iceberg-rust]
DerGut opened a new pull request, #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670
## Which issue does this PR close?
Closes #1669
## What changes are included in this PR?
`TableIdent` now derives `PartialOrd`.
## Are these changes tested?
No, it's only derived behavior. I'm happy to add a test, but I'm not 100% convinced it would add value.
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670#discussion_r2358131385
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -225,7 +225,7 @@ impl Display for NamespaceIdent {
}
/// TableIdent represents the identifier of a table in the catalog.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
Review Comment:
We need to derive both `PartialOrd` and `Ord`. Deriving `Ord` only will not work.
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670#discussion_r2351826054
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -225,7 +225,7 @@ impl Display for NamespaceIdent {
}
/// TableIdent represents the identifier of a table in the catalog.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
Review Comment:
Should we also derive `Ord`? I think `TableIdent` supports full order.
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670#discussion_r2354952126
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -225,7 +225,7 @@ impl Display for NamespaceIdent {
}
/// TableIdent represents the identifier of a table in the catalog.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
Review Comment:
[`NamespaceIdent`](https://github.com/apache/iceberg-rust/blob/e7db4374e4df983bd63a617380ae215d66f271c2/crates/iceberg/src/catalog/mod.rs#L130) does 👍
Given there's no pushback to adding ordering in general, I agree that `Ord` represents it more accurately.
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670#discussion_r2354974657
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -225,7 +225,7 @@ impl Display for NamespaceIdent {
}
/// TableIdent represents the identifier of a table in the catalog.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
Review Comment:
Should we update the PR?
```suggestion
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Ord, Hash)]
```
--
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: TableIdent derives PartialOrd [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #1670:
URL: https://github.com/apache/iceberg-rust/pull/1670#discussion_r2366227082
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -225,7 +225,7 @@ impl Display for NamespaceIdent {
}
/// TableIdent represents the identifier of a table in the catalog.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
Review Comment:
Yep, sorry I didn't get to fixing it last week. Latest commit should do it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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