You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/07/26 09:22:40 UTC
[GitHub] [avro] clesaec opened a new pull request, #1787: Avro 3532 naming rules
clesaec opened a new pull request, #1787:
URL: https://github.com/apache/avro/pull/1787
[AVRO-3532](https://issues.apache.org/jira/browse/AVRO-3532) : Documentation give strict naming rules for field name, and Java implementation allow more than this (it allows accents : "éhôÄç", chinese alphabet : "歳以上")
This PR aims to formalize the rules to officially allow this kind of names (by adding a Unit Test in Java and update rust code to accept it (with a unit test as well).
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
clesaec commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929962748
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
Review Comment:
ok
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
Review Comment:
ok
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929801124
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
Review Comment:
```suggestion
if §name.len() == 0 {
```
`i1` is a bad name anyway
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
Review Comment:
cache the value of `name.chars()` and reuse it
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
Review Comment:
no need to iterate if `result` is `false`
##########
lang/rust/avro/src/schema.rs:
##########
@@ -3818,4 +3833,12 @@ mod tests {
panic!("Expected Schema::Record");
}
}
+
+ #[test]
+ fn validate_name_test() {
+ let p = Parser::default();
+ let r = p.validate_name(&"歳以上".to_owned());
+ assert_eq!(r, true);
Review Comment:
```suggestion
assert!(r);
```
##########
lang/rust/avro/src/schema.rs:
##########
@@ -3818,4 +3833,12 @@ mod tests {
panic!("Expected Schema::Record");
}
}
+
+ #[test]
+ fn validate_name_test() {
Review Comment:
```suggestion
fn avro_3532_validate_name() {
```
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
+ result &= name.chars().nth(i).unwrap().is_alphanumeric();
+ }
+ return result;
Review Comment:
Actually I think you can simplify the last 6 lines to `name.chars().any(|c| !c.is_alphabetic())` (implicit return)
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1787: AVRO-3532: Naming rules
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #1787:
URL: https://github.com/apache/avro/pull/1787#issuecomment-1303941984
I took the liberty of changing these two PRs (https://github.com/apache/avro/pull/1787 and https://github.com/apache/avro/pull/1798) to draft status, just to prevent any accidents!
These behaviour changes should be merged after a change to the specification, and we really should have a stronger consensus around the whether this is the right thing to do before changing the spec.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929807777
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
+ result &= name.chars().nth(i).unwrap().is_alphanumeric();
+ }
+ return result;
Review Comment:
Actually I think you can simplify the method to `name.chars().any(|c| !c.is_alphabetic())` (implicit return)
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929970517
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
+ result &= name.chars().nth(i).unwrap().is_alphanumeric();
+ }
+ return result;
Review Comment:
would that work `name.chars().drop(1).all(|c| c.is_alphabetic())` ?
I haven't tried 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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
clesaec commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929964047
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
+ result &= name.chars().nth(i).unwrap().is_alphanumeric();
+ }
+ return result;
Review Comment:
how to distinguish first and others ? need to have a kind of "indexedAny" ?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
clesaec commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r930013379
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
+ return false;
+ }
+ let mut result : bool = name.chars().nth(0).unwrap().is_alphabetic();
+
+ let size = name.chars().count();
+ for i in 1..size {
+ result &= name.chars().nth(i).unwrap().is_alphanumeric();
+ }
+ return result;
Review Comment:
I finally found :
`name.char_indices().all(|(index, c)| ...)`
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1787: Avro 3532 naming rules
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1787:
URL: https://github.com/apache/avro/pull/1787#discussion_r929801124
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1282,6 +1280,23 @@ impl Parser {
Ok(schema)
}
+ fn validate_name(
+ &self,
+ name: &String
+ ) -> bool {
+ let i1 = name.len();
+ if i1 == 0 {
Review Comment:
```suggestion
if &name.len() == 0 {
```
`i1` is a bad name anyway
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3532: Naming rules [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #1787:
URL: https://github.com/apache/avro/pull/1787#issuecomment-1956655837
https://issues.apache.org/jira/browse/AVRO-3900 added an API to provide custom name validators to the Rust SDK.
By default it will use the specification rules but the user could provide custom impl if needed.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org