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 2021/02/16 16:18:31 UTC
[GitHub] [avro] icecreamhead opened a new pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
icecreamhead opened a new pull request #1097:
URL: https://github.com/apache/avro/pull/1097
Make sure you have checked _all_ steps below.
### Jira
- [x] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO-2872) issues and references them in the PR title. For example, "AVRO-2872: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-XXX
- In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
`TestSpecificCompiler#testLogicalTypeUnionProducesConversionField`
### Commits
- [x] 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. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] StephenFlavin commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
StephenFlavin commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-812168049
Raised AVRO-3102 (sorry for the noise in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on a change in pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1097:
URL: https://github.com/apache/avro/pull/1097#discussion_r612380027
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalType.java
##########
@@ -89,4 +91,36 @@ public void validate(Schema schema) {
}
}
+ /**
+ * Utility function to resolve the logical type from the given Schema
+ *
+ * @param schema a Schema representing a logical type
+ * @return the logical type represented by the schema, or null if the schema
+ * doesn't represent a logical type, or if the logical type is ambiguous
+ */
+ public static LogicalType resolveLogicalType(Schema schema) {
Review comment:
There's some utility methods [here](https://github.com/apache/avro/blob/ccbbb8353fcc168563c567580cb773d5fe442704/lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java#L64) for example. Perhaps this would be appropriately grouped with them, with a name like `fromSchemaIncludingNullable`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] StephenFlavin edited a comment on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
StephenFlavin edited a comment on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-811037661
I'm running into an issue which I think is related but building this PR locally and testing it hasn't fixed the issue.
I have a `schema.avdl`
```
@namespace("org.example")
protocol TestService {
record MaybeLogicalTypes {
union {null, date} dateType = null;
union {null, time_ms} timeMsType = null;
union {null, timestamp_ms} timestampMsType = null;
}
MaybeLogicalTypes getMaybeLogicalTypes();
}
```
I have server code which returns a `MaybeLogicalTypes` object with all the fields set however when the client code calls
```
SpecificRequestor.getClient(TestService.class, new HttpTransceiver(url)).getMaybeLogicalTypes()
```
I get an error when ` union {null, date}` or `union {null, time_ms}` or `union {null, timestamp_ms}` is populated.
e.g.
```
org.apache.avro.AvroRuntimeException: org.apache.avro.AvroRuntimeException: Unknown datum type java.time.Instant: 2021-03-31T11:26:20.123888Z
at org.apache.avro.ipc.specific.SpecificRequestor.readError(SpecificRequestor.java:160) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$Response.getResponse(Requestor.java:566) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:366) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:330) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Transceiver.transceive(Transceiver.java:73) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:152) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:101) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.specific.SpecificRequestor.invoke(SpecificRequestor.java:108) ~[avro-ipc-1.10.2.jar:1.10.2]
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-814881778
Good news: a `UNION` can't contain other unions, among other limitations (such as not containing two different logical types with the same underlying primitives, or even two `ARRAY` with different type contents.
It currently looks like an `ARRAY` of `timestamp-millis` does generate the methods with the correct type `List<Instant>`, but does not create the `conversions[]` array in the specific 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] StephenFlavin commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
StephenFlavin commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-811037661
I'm running into an issue which I think is related but building this PR locally and testing it hasn't fixed the issue.
I have a schema
```
record MaybeLogicalTypes {
union {null, date} dateType = null;
union {null, time_ms} timeMsType = null;
union {null, timestamp_ms} timestampMsType = null;
}
MaybeLogicalTypes getMaybeLogicalTypes();
```
I have server code which returns a `MaybeLogicalTypes` object with all the fields set however when the client code calls
```
SpecificRequestor.getClient(TestService.class, new HttpTransceiver(url)).getMaybeLogicalTypes()
```
I get an error when ` union {null, date}` or `union {null, time_ms}` or `union {null, timestamp_ms}` is populated.
e.g.
```
org.apache.avro.AvroRuntimeException: org.apache.avro.AvroRuntimeException: Unknown datum type java.time.Instant: 2021-03-31T11:26:20.123888Z
at org.apache.avro.ipc.specific.SpecificRequestor.readError(SpecificRequestor.java:160) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$Response.getResponse(Requestor.java:566) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:366) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:330) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Transceiver.transceive(Transceiver.java:73) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:152) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:101) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.specific.SpecificRequestor.invoke(SpecificRequestor.java:108) ~[avro-ipc-1.10.2.jar:1.10.2]
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] icecreamhead commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
icecreamhead commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-814725985
> Shouldn't LogicTypes be resolved recursively? Curious if it will be registered in the case of an Array of Logical Types
There's some discussion about this on the Jira. We've established that this fix works for a union of exactly two elements where one of them is null, but unions with more branches is still an open question. The current conversion resolver doesn't have flexibility to provide different implementations based on the runtime time. As an aside, are nested unions allowed? Even if they are, how are they functionally different from a single union with multiple elements?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on a change in pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1097:
URL: https://github.com/apache/avro/pull/1097#discussion_r612363711
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalType.java
##########
@@ -89,4 +91,36 @@ public void validate(Schema schema) {
}
}
+ /**
+ * Utility function to resolve the logical type from the given Schema
+ *
+ * @param schema a Schema representing a logical type
+ * @return the logical type represented by the schema, or null if the schema
+ * doesn't represent a logical type, or if the logical type is ambiguous
+ */
+ public static LogicalType resolveLogicalType(Schema schema) {
+ if (schema.getLogicalType() != null) {
+ return schema.getLogicalType();
+ } else if (schema.isUnion()) {
+ List<Schema> types = schema.getTypes();
+ if (types.size() == 1) {
+ // The union has a single branch that may or may not be a logical type - return
+ // it either way
+ return types.get(0).getLogicalType();
+ } else if (types.size() == 2) {
+ // The union has exactly two branches.
+ // If one branch is null and one is a logical type, return the logical type,
+ // else return neither
+ LogicalType t1 = types.get(0).getLogicalType();
+ LogicalType t2 = types.get(1).getLogicalType();
+ if (t1 != null && t2 == null) {
Review comment:
I don't think this is correct either!
If t1 is an `Schema.Type.STRING` and t2 is a logicalType `timestamp-millis`, for example, this won't return the right answer.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] icecreamhead commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
icecreamhead commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-816536720
This is ready for review following our discussions 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-785755806
Thanks for the contribution and your patience! I asked a question on the JIRA -- what symptoms are you seeing in this bug?
I'm wondering what would happen if passed a record with a UNION field that had two logical types (something compatible like a `date` and a `timestamp-millis`). Is it currently unusable, and what would change with this fix?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] StephenFlavin edited a comment on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
StephenFlavin edited a comment on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-811037661
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] StephenFlavin edited a comment on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
StephenFlavin edited a comment on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-811037661
I'm running into an issue which I think is related but building this PR locally and testing it hasn't fixed the issue.
I have a `schema.avdl`
```
record MaybeLogicalTypes {
union {null, date} dateType = null;
union {null, time_ms} timeMsType = null;
union {null, timestamp_ms} timestampMsType = null;
}
MaybeLogicalTypes getMaybeLogicalTypes();
```
I have server code which returns a `MaybeLogicalTypes` object with all the fields set however when the client code calls
```
SpecificRequestor.getClient(TestService.class, new HttpTransceiver(url)).getMaybeLogicalTypes()
```
I get an error when ` union {null, date}` or `union {null, time_ms}` or `union {null, timestamp_ms}` is populated.
e.g.
```
org.apache.avro.AvroRuntimeException: org.apache.avro.AvroRuntimeException: Unknown datum type java.time.Instant: 2021-03-31T11:26:20.123888Z
at org.apache.avro.ipc.specific.SpecificRequestor.readError(SpecificRequestor.java:160) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$Response.getResponse(Requestor.java:566) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:366) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor$TransceiverCallback.handleResult(Requestor.java:330) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Transceiver.transceive(Transceiver.java:73) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:152) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.Requestor.request(Requestor.java:101) ~[avro-ipc-1.10.2.jar:1.10.2]
at org.apache.avro.ipc.specific.SpecificRequestor.invoke(SpecificRequestor.java:108) ~[avro-ipc-1.10.2.jar:1.10.2]
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] icecreamhead commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
icecreamhead commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-818587514
Nudge @RyanSkraba
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-811866876
This looks related, but not exactly the same issue. This would fix the `conversions` array in the specific, generated `MaybeLogicalTypes`, but it looks like the IPC responder doesn't take it into account in any case. Can you create a new JIRA?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] Fokko commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
Fokko commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-814340640
Shouldn't LogicTypes be resolved recursively? Curious if it will be registered in the case of an Array of Logical Types
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] icecreamhead commented on pull request #1097: AVRO-2872: Fix bug in SpecificCompiler which prevents Logical Types being registered for union fields
Posted by GitBox <gi...@apache.org>.
icecreamhead commented on pull request #1097:
URL: https://github.com/apache/avro/pull/1097#issuecomment-785204857
What do I need to do to get eyes on this? @RyanSkraba ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org