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/10/19 22:40:48 UTC
[GitHub] [avro] Danny02 opened a new pull request, #1919: AVRO-3649: reorder union types to match default
Danny02 opened a new pull request, #1919:
URL: https://github.com/apache/avro/pull/1919
Why:
We want to use @AvroDefault with @Nullable.
How does it help with resolving the issue:
The schema generation automaticly picks the right order of the union type.
Side effects:
This change has no side effects, because:
- The order of union types is irrelevant for schema compability.
- This change enables definitions wich has been invalid.
## What is the purpose of the change
Implementing AVRO-3649.
## Verifying this change
- *Added test that validates that a correct schema is generated*
## Documentation
- **This is my big question. Is the reflection serialization documented anywhere?**
--
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 commented on pull request #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #1919:
URL: https://github.com/apache/avro/pull/1919#issuecomment-1285933652
> This is my big question. Is the reflection serialization documented anywhere?
The best documentation for the reflection annotations is currently the Javadoc. Hopefully the new website will draw contributors to improve this situation...
--
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] Danny02 commented on a diff in pull request #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
Danny02 commented on code in PR #1919:
URL: https://github.com/apache/avro/pull/1919#discussion_r1000516672
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}
+ /**
+ * Checks if a JSON value matches the schema.
+ *
+ * @param jsonValue a value to check against the schema
+ * @return true if the value is valid according to this schema
+ */
+ public boolean isValidValue(JsonNode jsonValue) {
+ return isValidDefault(this, jsonValue);
+ }
Review Comment:
Just was able to take a look at your PR. It looks like a even better solution, because it "fixes" the problem on a lower level.
Would you say that this PR is now obsolete?
--
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] Danny02 commented on a diff in pull request #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
Danny02 commented on code in PR #1919:
URL: https://github.com/apache/avro/pull/1919#discussion_r1000456532
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}
+ /**
+ * Checks if a JSON value matches the schema.
+ *
+ * @param jsonValue a value to check against the schema
+ * @return true if the value is valid according to this schema
+ */
+ public boolean isValidValue(JsonNode jsonValue) {
+ return isValidDefault(this, jsonValue);
+ }
Review Comment:
thx for the feedback, can u give me some points what `standard method` you are talking about?
--
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 #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
clesaec commented on code in PR #1919:
URL: https://github.com/apache/avro/pull/1919#discussion_r1000534983
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}
+ /**
+ * Checks if a JSON value matches the schema.
+ *
+ * @param jsonValue a value to check against the schema
+ * @return true if the value is valid according to this schema
+ */
+ public boolean isValidValue(JsonNode jsonValue) {
+ return isValidDefault(this, jsonValue);
+ }
Review Comment:
standard method => i would say "non static overridable method" (better for object programming).
For 2 PR of 2 solution, I think we should discuss about that on the JIRA issue, i may missed some point (i want advice of other developer).
--
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 #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
clesaec commented on code in PR #1919:
URL: https://github.com/apache/avro/pull/1919#discussion_r1000403327
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}
+ /**
+ * Checks if a JSON value matches the schema.
+ *
+ * @param jsonValue a value to check against the schema
+ * @return true if the value is valid according to this schema
+ */
+ public boolean isValidValue(JsonNode jsonValue) {
+ return isValidDefault(this, jsonValue);
+ }
Review Comment:
:+1: use standard method instead of static (_it may replace private static boolean isValidDefault in near future_), and avoid this long switch/case on schema/type.
About default value on union type, I wonder if it couldn't be even better to change logic and get the first compatible Schema of union with the value (And throw exception if none) ?
I did this code in local (_and partially copy yours_), and it seems to work fine (at least for unit test). I will put more details in linked JIRA Ticket in few minutes.
--
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 #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #1919:
URL: https://github.com/apache/avro/pull/1919#discussion_r1000924833
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}
+ /**
+ * Checks if a JSON value matches the schema.
+ *
+ * @param jsonValue a value to check against the schema
+ * @return true if the value is valid according to this schema
+ */
+ public boolean isValidValue(JsonNode jsonValue) {
+ return isValidDefault(this, jsonValue);
+ }
Review Comment:
Is there any possible way we can rewrite this without adding or exposing the `isValidValue(JsonNode ...)` method?
There was a reason the isValidDefault method is private: we **really** don't want to be exposing the jackson internals in public methods and (if I remember correctly). There was a fair amount of work deleting these methods in the past, in order to _one day_ replace the jackson internals.
If anyone has a better memory than me, please feel to chime in!
--
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 #1919: AVRO-3649: reorder union types to match default
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #1919:
URL: https://github.com/apache/avro/pull/1919#issuecomment-1285920936
Just to link these two ideas together: @clesaec 's proposal is at https://github.com/apache/avro/pull/1922
--
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