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/11/23 15:53:55 UTC
[GitHub] [avro] opwvhk opened a new pull request #1411: AVRO-3257: IDL support for nullable types
opwvhk opened a new pull request #1411:
URL: https://github.com/apache/avro/pull/1411
Make sure you have checked _all_ steps below.
### Jira
- [X] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3257
- ~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~:
-- Adjusted the test input `simple.avdl`, but not the output `simple.avpr` (the result should be identical to the original)
### 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"
**NOTE**: This PR is intended to be merged after PR #1407 for AVRO-3256 (it includes changes required by this PR). To review, please only check the last commit.
### Documentation
- [X] 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
- The website documentation has been extended to include the new functionality
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r755274774
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -1444,15 +1443,42 @@ Schema UnannotatedType(Map<String, JsonNode> props):
{
Schema s;
}
+{
+ (
+ s = NullableType(props)
+ | (
+ s = UnionDefinition()
+ | s = ArrayType()
+ | s = MapType()
+ )
+ {
+ // NullableType also applies properties, inside any union with null it may create.
+ for (String key : props.keySet())
+ Accessor.addProp(s, key, props.get(key));
+ }
+ )
+ {
+ return s;
+ }
+}
+
+Schema NullableType(Map<String, JsonNode> props):
+{
+ Schema s;
+ boolean optional = false;
+}
{
(
s = ReferenceType() { if (!props.isEmpty()) { throw error("Type references may not be annotated", token); } }
| s = PrimitiveType()
- | s = UnionDefinition()
- | s = ArrayType()
- | s = MapType()
- )
+ ) [ <QUESTION_MARK> { optional = true; } ]
{
+ // By applying the properties here (before creating the union), type annotations modify the optional type instead of the union.
+ for (String key : props.keySet())
+ Accessor.addProp(s, key, props.get(key));
+ if (optional) {
+ s = Schema.createUnion(Schema.create(Schema.Type.NULL), s);
+ }
Review comment:
This is the new bit: reference & primitive/logical types are created as usual (including properties), and if the type is intended as optional, wrapped in a union with null.
The duplicated code to apply properties is intentional: if merged with the code in the `UnannotatedType` production, the properties would be applied to the union, instead of the optional primitive type.
--
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] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on pull request #1411:
URL: https://github.com/apache/avro/pull/1411#issuecomment-976851472
The failed check complains about the generated parser code (and says the same error is fixed in another line).
My conclusion: this is a false positive.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r764002710
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -1444,15 +1443,42 @@ Schema UnannotatedType(Map<String, JsonNode> props):
{
Schema s;
}
+{
+ (
+ s = NullableType(props)
+ | (
+ s = UnionDefinition()
+ | s = ArrayType()
+ | s = MapType()
+ )
+ {
+ // NullableType also applies properties, inside any union with null it may create.
+ for (String key : props.keySet())
+ Accessor.addProp(s, key, props.get(key));
+ }
+ )
+ {
+ return s;
+ }
+}
+
+Schema NullableType(Map<String, JsonNode> props):
+{
+ Schema s;
+ boolean optional = false;
+}
{
(
s = ReferenceType() { if (!props.isEmpty()) { throw error("Type references may not be annotated", token); } }
| s = PrimitiveType()
- | s = UnionDefinition()
- | s = ArrayType()
- | s = MapType()
- )
+ ) [ <QUESTION_MARK> { optional = true; } ]
{
+ // By applying the properties here (before creating the union), type annotations modify the optional type instead of the union.
+ for (String key : props.keySet())
+ Accessor.addProp(s, key, props.get(key));
+ if (optional) {
+ s = Schema.createUnion(Schema.create(Schema.Type.NULL), s);
+ }
Review comment:
Note: when AVRO-2976 (PR #985) is merged, this need to be adjusted.
Conversely, if this PR is merged first, #985 needs to be adjusted.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r755275386
##########
File path: lang/java/compiler/src/test/idl/input/simple.avdl
##########
@@ -60,7 +60,7 @@ protocol Simple {
time_ms t = 0;
@foo.bar("bar.foo") long l = 0;
- union {null, @foo.foo.bar(42) @foo.foo.foo("3foo") string} prop = null;
+ @foo.foo.bar(42) @foo.foo.foo("3foo") string? prop = null;
Review comment:
This change demonstrates the new functionality. `simple.avpr` has not changed, as the result must be identical.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r764192266
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -90,6 +90,7 @@ import org.apache.commons.lang3.StringEscapeUtils;
*/
public class Idl implements Closeable {
static JsonNodeFactory FACTORY = JsonNodeFactory.instance;
+ private static final String OPTIONAL_NULLABLE_TYPE_PROPERTY = "org.apache.avro.compiler.idl.Idl.NullableType.optional";
Review comment:
This field is used as marker: `NullableType()` adds it, and `fixOptionalSchema` _always_ removes it.
##########
File path: doc/src/content/xdocs/idl.xml
##########
@@ -301,17 +301,36 @@ record Card {
<section id="unions">
<title>Unions</title>
<p>Union types are denoted as <code>union { typeA, typeB, typeC, ... }</code>. For example,
- this record contains a string field that is optional (unioned with <code>null</code>):
+ this record contains a string field that is optional (unioned with <code>null</code>), and
+ a field containing either a precise or a imprecise number:
</p>
<source>
record RecordWithUnion {
union { null, string } optionalString;
+ union { decimal(12, 6), float } number;
}
</source>
<p>
Note that the same restrictions apply to Avro IDL unions as apply to unions defined in the
- JSON format; namely, a record may not contain multiple elements of the same type.
+ JSON format; namely, a record may not contain multiple elements of the same type. Also,
+ fields/parameters that use the union type and have a default parameter must specify a
+ default value of the same type as the <em>first</em>em> union type.
</p>
+ <p>Because it occurs so often, there is a special shorthand to denote a union of
+ <code>null</code> with another type. In the following snippet, the first three fields have
+ identical types:
+ </p>
+ <source>
+record RecordWithUnion {
+ union { null, string } optionalString1 = null;
+ string? optionalString2 = null;
+ string? optionalString3; // No default value
+ string? optionalString4 = "something";
+}
+ </source>
+ <p>Note that unlike explicit unions, the position of the <code>null</code> type is fluid; it will be
+ the first or last type depending on the default value (if any). So in the example above, all fields
+ are valid.</p>
Review comment:
These last two fields and the "fluid" unions are a recent addition; this makes the '?' syntax more useful.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r755272274
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -1444,15 +1443,42 @@ Schema UnannotatedType(Map<String, JsonNode> props):
{
Schema s;
}
+{
+ (
+ s = NullableType(props)
Review comment:
This is what makes the magic possible:
1. Primitive/logical & reference types are handled differently (elsewhere)
2. The other types are handled as usual
--
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 change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r766092460
##########
File path: lang/java/compiler/src/test/idl/input/simple.avdl
##########
@@ -60,7 +60,7 @@ protocol Simple {
time_ms t = 0;
@foo.bar("bar.foo") long l = 0;
- union {null, @foo.foo.bar(42) @foo.foo.foo("3foo") string} prop = null;
+ @foo.foo.bar(42) @foo.foo.foo("3foo") string? prop = null;
Review comment:
IMO it would be good to add some comments next to the modified fields, for the future reader/you.
E.g. next to `time_ms? t = 0` we could add a comment like: `demonstrates a union schema with nonNull default value, so the nonNull schema is first`.
And next to `string? prop = null;` we could add a comment like: `demonstrates a union schema with null default value, so the nonNull schema is second`
--
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 change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r773355163
##########
File path: doc/src/content/xdocs/idl.xml
##########
@@ -309,17 +309,36 @@ record Card {
<section id="unions">
<title>Unions</title>
<p>Union types are denoted as <code>union { typeA, typeB, typeC, ... }</code>. For example,
- this record contains a string field that is optional (unioned with <code>null</code>):
+ this record contains a string field that is optional (unioned with <code>null</code>), and
+ a field containing either a precise or a imprecise number:
</p>
<source>
record RecordWithUnion {
union { null, string } optionalString;
+ union { decimal(12, 6), float } number;
}
</source>
<p>
Note that the same restrictions apply to Avro IDL unions as apply to unions defined in the
- JSON format; namely, a record may not contain multiple elements of the same type.
+ JSON format; namely, a record may not contain multiple elements of the same type. Also,
+ fields/parameters that use the union type and have a default parameter must specify a
+ default value of the same type as the <em>first</em>em> union type.
Review comment:
```suggestion
default value of the same type as the <em>first</em> union type.
```
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r767190101
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
Nothing too magical; I can check for schemas of null with non-null: `schema.isUnion() && schema.getTypes().size() == 2 && schema.getTypes().get(0).isNullable() != schema.getTypes().get(1).isNullable()`
And then swap the union if: `defaultValue != null && !defaultValue.isNull() && schema.getTypes().get(0).isNullable()`
However, I can also do this in the Schema.Field constructor.
Or even combine it with `org.apache.avro.Schema#isValidDefault(Schema, JsonNode)` to select the first schema for which the default is valid, and place it as the first schema in the union.
I've posted a poll to the dev mailinglist to choose between the options.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r767190931
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
The main problem is a) union instances must still have the default value match the first schema (I don't know enough to relax that constraint), and b) the union will not be what was specified.
As a result, my preference is to restrict this feature to the `?` syntax.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r767190101
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
Nothing too magical; I can check for schemas of null with non-null: `schema.isUnion() && schema.getTypes().size() == 2 && schema.getTypes().get(0).isNullable() != schema.getTypes().get(1).isNullable()`
And then swap the union if: `defaultValue != null && !defaultValue.isNull() && schema.getTypes().get(0).isNullable()`
However, I can also do this in the Schema.Field constructor.
Or even combine it with `org.apache.avro.Schema#isValidDefault(Schema, JsonNode)` to select the first schema for which the default is valid, and place it as the first schema in the union.
I've posted a poll to the dev mailinglist to choose between the options.
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
The main problem is a) union instances must still have the default value match the first schema (I don't know enough to relax that constraint), and b) the union will not be what was written.
As a result, my preference is to restrict this feature to the `?` syntax.
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
The main problem is a) union instances must still have the default value match the first schema (I don't know enough to relax that constraint), and b) the union will not be what was specified.
As a result, my preference is to restrict this feature to the `?` syntax.
--
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] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on pull request #1411:
URL: https://github.com/apache/avro/pull/1411#issuecomment-991781519
There are now still three PRs open (this one, #1377 and #1412; all by me) that touch the file `idl.jj`, and that slightly conflict with each other.
I suggest that if all three meet approval, I merge them all into this PR. Is that a good idea?
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r767190931
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
The main problem is a) union instances must still have the default value match the first schema (I don't know enough to relax that constraint), and b) the union will not be what was written.
As a result, my preference is to restrict this feature to the `?` syntax.
--
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 change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r766035699
##########
File path: lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
##########
@@ -182,6 +183,32 @@ public class Idl implements Closeable {
return result;
}
+ /**
+ * For "optional schemas" (recognized by the marker property the NullableType
+ * production adds), ensure the null schema is in the right place.
+ *
+ * @param schema a schema
+ * @param defaultValue the intended default value
+ * @return the schema, or an optional schema with null in the right place
+ */
+ private static Schema fixOptionalSchema(Schema schema, JsonNode defaultValue) {
Review comment:
I like this so much! This is a compelling feature... I'm tempted to say that nobody really **_wants_**
```
union { null, double } value = NaN
```
to fail either... What would you think about auto-correcting all `union { null, notNull }` or `union { notNull, null } ` types that have a default?
I guess this might be too magical, and i suspect that the `?` syntax will be happily adopted in any case.
--
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 change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r773355163
##########
File path: doc/src/content/xdocs/idl.xml
##########
@@ -309,17 +309,36 @@ record Card {
<section id="unions">
<title>Unions</title>
<p>Union types are denoted as <code>union { typeA, typeB, typeC, ... }</code>. For example,
- this record contains a string field that is optional (unioned with <code>null</code>):
+ this record contains a string field that is optional (unioned with <code>null</code>), and
+ a field containing either a precise or a imprecise number:
</p>
<source>
record RecordWithUnion {
union { null, string } optionalString;
+ union { decimal(12, 6), float } number;
}
</source>
<p>
Note that the same restrictions apply to Avro IDL unions as apply to unions defined in the
- JSON format; namely, a record may not contain multiple elements of the same type.
+ JSON format; namely, a record may not contain multiple elements of the same type. Also,
+ fields/parameters that use the union type and have a default parameter must specify a
+ default value of the same type as the <em>first</em>em> union type.
Review comment:
```suggestion
default value of the same type as the <em>first</em> union type.
```
Probably a mistake here.
--
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] opwvhk commented on a change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r755269337
##########
File path: doc/src/content/xdocs/idl.xml
##########
@@ -301,17 +301,32 @@ record Card {
<section id="unions">
<title>Unions</title>
<p>Union types are denoted as <code>union { typeA, typeB, typeC, ... }</code>. For example,
- this record contains a string field that is optional (unioned with <code>null</code>):
+ this record contains a string field that is optional (unioned with <code>null</code>), and
+ a field containing either a precise or a imprecise number:
</p>
<source>
record RecordWithUnion {
union { null, string } optionalString;
+ union { decimal(12, 6), float } number;
}
</source>
<p>
Note that the same restrictions apply to Avro IDL unions as apply to unions defined in the
- JSON format; namely, a record may not contain multiple elements of the same type.
+ JSON format; namely, a record may not contain multiple elements of the same type. Also,
+ fields/parameters that use the union type and have a default parameter must specify a
+ default value of the same type as the <em>first</em>em> union type.
</p>
+ <p>Because it occurs so often, there is a special shorthand to denote a union of
+ <code>null</code> with another type. The following two fields have identical types:
+ </p>
+ <source>
+record RecordWithUnion {
+ union { null, string } optionalString1;
+ string? optionalString2;
+}
+ </source>
+ <p>Note that the union contains the <code>null</code> type first, so the only allowed default
+ value is <code>null</code>.</p>
Review comment:
This documents the addition
--
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 #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1411:
URL: https://github.com/apache/avro/pull/1411#issuecomment-991884067
Ooof -- my apologies, I merged #1377 before I read your comment and caused a merge conflict :/ I'll try to get #1412 reviewed as soon as possible but I'll confirm with you before merging (so you don't have to fix the merge conflicts twice).
--
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 change in pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1411:
URL: https://github.com/apache/avro/pull/1411#discussion_r766084596
##########
File path: doc/src/content/xdocs/idl.xml
##########
@@ -301,17 +301,36 @@ record Card {
<section id="unions">
<title>Unions</title>
<p>Union types are denoted as <code>union { typeA, typeB, typeC, ... }</code>. For example,
- this record contains a string field that is optional (unioned with <code>null</code>):
+ this record contains a string field that is optional (unioned with <code>null</code>), and
+ a field containing either a precise or a imprecise number:
</p>
<source>
record RecordWithUnion {
union { null, string } optionalString;
+ union { decimal(12, 6), float } number;
}
</source>
<p>
Note that the same restrictions apply to Avro IDL unions as apply to unions defined in the
- JSON format; namely, a record may not contain multiple elements of the same type.
+ JSON format; namely, a record may not contain multiple elements of the same type. Also,
+ fields/parameters that use the union type and have a default parameter must specify a
+ default value of the same type as the <em>first</em>em> union type.
</p>
+ <p>Because it occurs so often, there is a special shorthand to denote a union of
+ <code>null</code> with another type. In the following snippet, the first three fields have
+ identical types:
+ </p>
+ <source>
+record RecordWithUnion {
+ union { null, string } optionalString1 = null;
+ string? optionalString2 = null;
+ string? optionalString3; // No default value
+ string? optionalString4 = "something";
+}
+ </source>
+ <p>Note that unlike explicit unions, the position of the <code>null</code> type is fluid; it will be
+ the first or last type depending on the default value (if any). So in the example above, all fields
+ are valid.</p>
Review comment:
We should not forget to update the new website once it starts being used (https://lists.apache.org/thread/ktjh7kyd92hf0jsmpp16c3k121dc8grg)
--
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: [GitHub] [avro] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by Martin Grigorov <mg...@apache.org>.
I think it would be better if you leave them three and be merged one by one.
The benefits are:
* better Git history
* easier to identify regressions in smaller diffs and revert the
problematic one if needed
My 2c.
On Sat, Dec 11, 2021, 22:56 GitBox <gi...@apache.org> wrote:
>
> opwvhk commented on pull request #1411:
> URL: https://github.com/apache/avro/pull/1411#issuecomment-991781519
>
>
> There are now still three PRs open (this one, #1377 and #1412; all by
> me) that touch the file `idl.jj`, and that slightly conflict with each
> other.
>
> I suggest that if all three meet approval, I merge them all into this
> PR. Is that a good idea?
>
>
> --
> 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: [GitHub] [avro] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by Martin Grigorov <mg...@apache.org>.
I think it would be better if you leave them three and be merged one by one.
The benefits are:
* better Git history
* easier to identify regressions in smaller diffs and revert the
problematic one if needed
My 2c.
On Sat, Dec 11, 2021, 22:56 GitBox <gi...@apache.org> wrote:
>
> opwvhk commented on pull request #1411:
> URL: https://github.com/apache/avro/pull/1411#issuecomment-991781519
>
>
> There are now still three PRs open (this one, #1377 and #1412; all by
> me) that touch the file `idl.jj`, and that slightly conflict with each
> other.
>
> I suggest that if all three meet approval, I merge them all into this
> PR. Is that a good idea?
>
>
> --
> 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] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on pull request #1411:
URL: https://github.com/apache/avro/pull/1411#issuecomment-991781519
There are now still three PRs open (this one, #1377 and #1412; all by me) that touch the file `idl.jj`, and that slightly conflict with each other.
I suggest that if all three meet approval, I merge them all into this PR. Is that a good idea?
--
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 merged pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1411:
URL: https://github.com/apache/avro/pull/1411
--
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] opwvhk commented on pull request #1411: AVRO-3257: IDL support for nullable types
Posted by GitBox <gi...@apache.org>.
opwvhk commented on pull request #1411:
URL: https://github.com/apache/avro/pull/1411#issuecomment-999692180
Perfect merge of master; LGTM!
--
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