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/08/29 11:30:43 UTC
[GitHub] [avro] jjaakola-aiven opened a new pull request, #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
jjaakola-aiven opened a new pull request, #1843:
URL: https://github.com/apache/avro/pull/1843
Python implementation does not treat the name and namespace element as Java library.
This causes the compatibility check to fail as the name which can be fully qualified
is not splitted to name and namespace elements.
### Jira
https://issues.apache.org/jira/browse/AVRO-3622
### Tests
- [X] My PR adds the following unit tests
- Java: test case to `COMPATIBLE_READER_WRITER_TEST_CASES`, to demonstrate that Java library does not have the issue described.
- Python: same test case to Python lib.
### Commits
- [] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. [X] Subject is separated from body by a blank line
1. [] Subject is limited to 50 characters (not including Jira issue reference)
1. [X] Subject does not end with a period
1. [X] Subject uses the imperative mood ("add", not "adding")
1. [X] Body wraps at 72 characters
1. [X] Body explains "what" and "why", not "how"
--
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] RyanSkraba merged pull request #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged PR #1843:
URL: https://github.com/apache/avro/pull/1843
--
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] jjaakola-aiven commented on pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
jjaakola-aiven commented on PR #1843:
URL: https://github.com/apache/avro/pull/1843#issuecomment-1232606778
@martin-g I don't have enough insight if the Rust impl is correct. Like @perj notes the null namespace is not valid for dot separated names per specification. I think this is up for Avro maintainers to decide if `.record` is a valid or invalid name.
Java accepts this `new Schema.Name(".record", null);` Seems Rust accepts also. Python does not. Please note that I did not change the validation functionality of the name in this PR, I added a test for case `.record`. I just tested with Python lib version `1.11.0` and it raises also `avro.errors.InvalidName`. So this behavior was already in Python lib.
--
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 #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r958268249
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
name.py line 83 says ` # The empty string may be used as a namespace to indicate the null namespace.`
The spec [says](https://avro.apache.org/docs/1.11.1/specification/#names) the same.
--
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] jjaakola-aiven commented on a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
jjaakola-aiven commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r958380963
##########
lang/py/avro/test/test_compatibility.py:
##########
@@ -659,6 +688,7 @@ def test_schema_compatibility(self):
ENUM_ABC_FIELD_DEFAULT_B_ENUM_DEFAULT_A_RECORD,
),
(NS_RECORD1, NS_RECORD2),
+ (WITHOUT_NAMESPACE_RECORD, WITH_NAMESPACE_RECORD),
Review Comment:
From the specification, https://avro.apache.org/docs/1.11.1/specification/_print/#schema-resolution
```
* It is an error if the two schemas do not match. To match, one of the following must hold:
...
* both schemas are records with the same (unqualified) name
...
```
I am assuming this holds for compatibility.
This change also aligns Python lib behavior with the Java implementation.
The compatibility of names is handled on this line in Python lib: https://github.com/apache/avro/blob/master/lang/py/avro/compatibility.py#L378
In Java lib it is here: https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/SchemaCompatibility.java#L97
It could be error in the specification and therefore in the implementation that namespace is not relevant.
--
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] jjaakola-aiven commented on a diff in pull request #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
jjaakola-aiven commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960984260
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
Fixed.
--
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 a diff in pull request #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960936350
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, "o..a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
Review Comment:
Repeated test? How about `"a."` for completeness!
--
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 #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960548811
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
Schemata with `"name": "Abc"` or `"name": ".Abc"`, both without explicit `"namespace"` would have the same [full]names.
--
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] KalleOlaviNiemitalo commented on pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1843:
URL: https://github.com/apache/avro/pull/1843#issuecomment-1232682145
I don't know much about Python so won't review this.
In general, I would like more test data to be shared between language implementations, to detect interoperability problems. Such a change seems out of scope for this PR 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@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 #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r961089442
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
> That can of worms needs to be solved but not necessarily in this PR...
Agreed!
--
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 pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1843:
URL: https://github.com/apache/avro/pull/1843#issuecomment-1232623152
https://github.com/apache/avro/pull/1843/files#diff-f13709b70a228170658d5db1f82fca6cf61a1968f63c6081be6cc2eeb6334973R84 is a new code that does some Python magic (`.[-1]`, i.e. returns the last chunk) and that's why I asked for more tests.
Let's wait some more for other devs to also take a look at the 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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] perj commented on a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
perj commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r958554420
##########
lang/py/avro/test/test_compatibility.py:
##########
@@ -659,6 +688,7 @@ def test_schema_compatibility(self):
ENUM_ABC_FIELD_DEFAULT_B_ENUM_DEFAULT_A_RECORD,
),
(NS_RECORD1, NS_RECORD2),
+ (WITHOUT_NAMESPACE_RECORD, WITH_NAMESPACE_RECORD),
Review Comment:
Thanks for explaining, it does look like this is per the spec then.
--
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] lebaptiste commented on a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
lebaptiste commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960544168
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
> A few lines later, the spec says:
>
> > The null namespace may not be used in a dot-separated sequence of names.
This is consistent with the canonical form detailed here: https://avro.apache.org/docs/1.11.1/specification/_print/#transforming-into-parsing-canonical-form
One would assume that a schema and its canonical form must have the same validation result?
--
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] jjaakola-aiven commented on a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
jjaakola-aiven commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r957326951
##########
lang/py/avro/name.py:
##########
@@ -91,6 +93,10 @@ def __eq__(self, other: object) -> bool:
"""Equality of names is defined on the fullname and is case-sensitive."""
return hasattr(other, "fullname") and self.fullname == getattr(other, "fullname")
+ @property
+ def name(self) -> Optional[str]:
Review Comment:
Added some.
--
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 pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1843:
URL: https://github.com/apache/avro/pull/1843#issuecomment-1232705787
> In general, I would like more test data to be shared between language implementations, to detect interoperability problems. Such a change seems out of scope for this PR though.
I'd be happy to work on the Rust part of such interop testing framework!
--
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 a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r959792937
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
This is currently under discussion on the mailing list -- I'm working through some issues with Java on `.MyRecord` style qualified names to see what the impact would be, but it's currently not permitted by 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] RyanSkraba commented on a diff in pull request #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960941587
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, "o..a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
Review Comment:
```suggestion
self.assertRaises(avro.errors.InvalidName, avro.schema.Name, "a.", None, None)
```
Looks like this is a repeated test, did you mean to test trailing dots?
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
```suggestion
# A name cannot start with dot.
```
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, "o..a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
Review Comment:
Repeated test? How about `"a."` for completeness!
--
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] jjaakola-aiven commented on a diff in pull request #1843: AVRO-3622: Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
jjaakola-aiven commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960983617
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, "o..a", None, None)
+ self.assertRaises(avro.errors.InvalidName, avro.schema.Name, ".a", None, None)
Review Comment:
Yes I did. Thank you.
--
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 #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960551021
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
> The null namespace may not be used in a dot-separated sequence of names.
I think the idea behind this text is that `"name": ".some.ns.Abc"` is not allowed. I.e. one cannot use the `null` namespace when there is more in the namespace part of the name.
--
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 a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r960619608
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
To be very clear : `.Abc` is not currently a supported fullname. Best practice strongly suggests that you don't use an unqualified name in a nested named schema. Today, you're likely to run into undefined and unexpected behaviour if you you do!
That can of worms needs to be solved but not necessarily in this 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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1843:
URL: https://github.com/apache/avro/pull/1843#issuecomment-1232580803
@jjaakola-aiven My only concern is https://github.com/apache/avro/pull/1843#discussion_r958268249
The Rust SDK supports it: https://github.com/apache/avro/blob/1943b173b9de00fa907d89bb11a98fbf8670fbb3/lang/rust/avro/src/schema.rs#L3065-L3069
--
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] perj commented on a diff in pull request #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
perj commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r958322538
##########
lang/py/avro/test/test_compatibility.py:
##########
@@ -659,6 +688,7 @@ def test_schema_compatibility(self):
ENUM_ABC_FIELD_DEFAULT_B_ENUM_DEFAULT_A_RECORD,
),
(NS_RECORD1, NS_RECORD2),
+ (WITHOUT_NAMESPACE_RECORD, WITH_NAMESPACE_RECORD),
Review Comment:
Feel free to ignore this as I'm not too familiar with this code, but are these two really compatible? To me, it reads like one of them has the null namespace and the other one has the namespace "ns". Don't namepaces have to match to be compatible?
I'd expect `{"namespace":"ns","name":"Record"}` to be compatible with `{"name":"ns.Record"}` but not too sure about if that's compatible with `{"name":"Record"}`. But like I said, I'm not an expert on Avro.
##########
lang/py/avro/test/test_schema.py:
##########
@@ -629,6 +641,10 @@ def test_invalid_name(self):
None,
None,
)
+ # A name cannot start with dot."
Review Comment:
A few lines later, the spec says:
> The null namespace may not be used in a dot-separated sequence of names.
--
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 #1843: AVRO-3622 (python) Fix compatibility check for schemas having or missing namespace
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1843:
URL: https://github.com/apache/avro/pull/1843#discussion_r957262126
##########
lang/py/avro/name.py:
##########
@@ -91,6 +93,10 @@ def __eq__(self, other: object) -> bool:
"""Equality of names is defined on the fullname and is case-sensitive."""
return hasattr(other, "fullname") and self.fullname == getattr(other, "fullname")
+ @property
+ def name(self) -> Optional[str]:
Review Comment:
Let's add some unit tests for this new method with inputs like:
1) "Record"
2) "ns.Record"
3) "longer.ns.Record"
4) ".Record"
--
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