You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "AndreMouche (via GitHub)" <gi...@apache.org> on 2024/08/08 08:48:31 UTC
[PR] arrow/schema.rs: refactor tests [iceberg-rust]
AndreMouche opened a new pull request, #531:
URL: https://github.com/apache/iceberg-rust/pull/531
Hello,
I found the test code a bit difficult to read while trying to understand the logic process of the arrow part, so I did some simple refactoring.
--
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] arrow/schema.rs: refactor tests [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #531:
URL: https://github.com/apache/iceberg-rust/pull/531
--
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] arrow/schema.rs: refactor tests [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #531:
URL: https://github.com/apache/iceberg-rust/pull/531#discussion_r1711072296
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -649,178 +649,106 @@ mod tests {
use super::*;
use crate::spec::Schema;
+ /**
+ * Create a simple field with metadata.
+ */
+ fn simple_field(name: &str, ty: DataType, nullable: bool, value: &str) -> Field {
+ Field::new(name, ty, nullable).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ value.to_string(),
+ )]))
+ }
+
fn arrow_schema_for_arrow_schema_to_schema_test() -> ArrowSchema {
let fields = Fields::from(vec![
- Field::new("key", DataType::Int32, false).with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "17".to_string(),
- )])),
- Field::new("value", DataType::Utf8, true).with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "18".to_string(),
- )])),
+ simple_field("key", DataType::Int32, false, "17"),
Review Comment:
+1, we can have a method to add single metadata.
--
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] arrow/schema.rs: refactor tests [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #531:
URL: https://github.com/apache/iceberg-rust/pull/531#issuecomment-2277296516
This PR looks good to me overall. I will make some room for @liurenjie1024 to take another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] arrow/schema.rs: refactor tests [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #531:
URL: https://github.com/apache/iceberg-rust/pull/531#discussion_r1709412928
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -649,178 +649,106 @@ mod tests {
use super::*;
use crate::spec::Schema;
+ /**
Review Comment:
Please use `///` for doc comments.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -649,178 +649,106 @@ mod tests {
use super::*;
use crate::spec::Schema;
+ /**
+ * Create a simple field with metadata.
+ */
+ fn simple_field(name: &str, ty: DataType, nullable: bool, value: &str) -> Field {
+ Field::new(name, ty, nullable).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ value.to_string(),
+ )]))
+ }
+
fn arrow_schema_for_arrow_schema_to_schema_test() -> ArrowSchema {
let fields = Fields::from(vec![
- Field::new("key", DataType::Int32, false).with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "17".to_string(),
- )])),
- Field::new("value", DataType::Utf8, true).with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "18".to_string(),
- )])),
+ simple_field("key", DataType::Int32, false, "17"),
Review Comment:
Hi @liurenjie1024, do you think it's a singal for us to refine the public API for `Field` along with parquet id?
--
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