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/02/25 20:01:16 UTC
[GitHub] [avro] KyleSchoonover opened a new pull request #1569: Avro 3421
KyleSchoonover opened a new pull request #1569:
URL: https://github.com/apache/avro/pull/1569
### Jira
- [ ] 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-3421
### Tests
- [ ] My PR does not need testing for this extremely good reason: Adds tests, and optimizes logic
### 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. 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.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KyleSchoonover edited a comment on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover edited a comment on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1051381652
Just wanted to call this out to @martin-g and @zcsizmadia. Is there a reason the ArraySchema class doesn't just use the following instead of the private constructor and the static NewInstance method?
`internal ArraySchema(JToken jtok, PropertyMap props, SchemaNames names, string encspace) : base(Type.Array, props)
{
}`
With the functionality of both put into this one constructor.
Also the addition of: `private ArraySchema() {}` so that it can't be instantiated without using the constructor.
--
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] KyleSchoonover commented on a change in pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on a change in pull request #1569:
URL: https://github.com/apache/avro/pull/1569#discussion_r815194963
##########
File path: lang/csharp/src/apache/test/Schema/ArraySchemaTests.cs
##########
@@ -0,0 +1,37 @@
+using NUnit.Framework;
Review comment:
Fixed
##########
File path: lang/csharp/src/apache/test/Schema/ArraySchemaTests.cs
##########
@@ -0,0 +1,37 @@
+using NUnit.Framework;
+
+namespace Avro.test
+{
+ [TestFixture]
+ public class ArraySchemaTests
+ {
+ [Test]
+ public void EqualsNullCheck()
+ {
+ string schemaString = "{\"type\": \"array\", \"items\": \"long\"}";
+ ArraySchema nullSchema = null;
+
+ Schema schema = Schema.Parse(schemaString);
+
+ if (schema is ArraySchema arraySchema)
+ {
+ Assert.False(arraySchema.Equals(nullSchema));
+ }
+ else
+ {
+ Assert.Fail("Schema was not an Array Schema");
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] martin-g commented on a change in pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1569:
URL: https://github.com/apache/avro/pull/1569#discussion_r815152865
##########
File path: lang/csharp/src/apache/test/Schema/ArraySchemaTests.cs
##########
@@ -0,0 +1,37 @@
+using NUnit.Framework;
Review comment:
You need to add the license header at the top of this new file. See ArraySchema.cs for example
##########
File path: lang/csharp/src/apache/test/Schema/ArraySchemaTests.cs
##########
@@ -0,0 +1,37 @@
+using NUnit.Framework;
+
+namespace Avro.test
+{
+ [TestFixture]
+ public class ArraySchemaTests
+ {
+ [Test]
+ public void EqualsNullCheck()
+ {
+ string schemaString = "{\"type\": \"array\", \"items\": \"long\"}";
+ ArraySchema nullSchema = null;
+
+ Schema schema = Schema.Parse(schemaString);
+
+ if (schema is ArraySchema arraySchema)
+ {
+ Assert.False(arraySchema.Equals(nullSchema));
+ }
+ else
+ {
+ Assert.Fail("Schema was not an Array Schema");
Review comment:
`Array Schema must not be equal to null` ?!
--
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] zcsizmadia commented on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052567195
@KyleSchoonover I totally support the idea if increasing the unit test coverage and I want to ake sure that most likely there is nothing wrong with this particular change.
The theoritical issue I am having is that changing the original library and new unit test for that change are happening at the same time. My immideate concern is that this is passing fine, however the change in the main library is a potential breaking change somewhere else for someb user downstream.
This is probably a good use case IMO to show why changing if conditions and casting and other non-trivial changes should be a low priority. And adding unit tests at the same time can potentially hide breaking changes.
--
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] KyleSchoonover commented on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052651276
@martin-g I removed the changes to ArraySchema, so that we can move that to a separate 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] KyleSchoonover edited a comment on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover edited a comment on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1051381652
Just wanted to call this out to @martin-g and @zcsizmadia. Is there a reason the ArraySchema class doesn't just use the following instead of the private constructor and the static NewInstance method?
`internal ArraySchema(JToken jtok, PropertyMap props, SchemaNames names, string encspace) : base(Type.Array, props)
{
}`
With the functionality of both put into this one constructor.
Also the addition of: `private ArraySchema(Schema items, PropertyMap props) : base(Type.Array, props) {}` so that it can't be instantiated without using the constructor.
I can put this in a separate PR, but just trying to understand if there was some intention that I don't see.
--
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] KyleSchoonover edited a comment on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover edited a comment on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1051381652
Just wanted to call this out to @martin-g and @zcsizmadia. Is there a reason the ArraySchema class doesn't just use the following instead of the private constructor and the static NewInstance method?
`internal ArraySchema(JToken jtok, PropertyMap props, SchemaNames names, string encspace) : base(Type.Array, props)
{
}`
With the functionality of both put into this one constructor.
Also the addition of: `private ArraySchema() {}` so that it can't be instantiated without using the constructor.
I can put this in a separate PR, but just trying to understand if there was some intention that I don't see.
--
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] KyleSchoonover commented on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1051381652
Just wanted to call this out to @martin-g and @zcsizmadia. Is there a reason the ArraySchema class doesn't just use the following instead of the private constructor and the static NewInstance method?
`internal ArraySchema(JToken jtok, PropertyMap props, SchemaNames names, string encspace) : base(Type.Array, props)
{
}`
With the functionality of both put into this one constructor.
--
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 merged pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #1569:
URL: https://github.com/apache/avro/pull/1569
--
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] KyleSchoonover commented on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052621437
@martin-g I've been following Extreme Programming for so long that it is rare that I implement code for potential changes (unless it's iterative development where I am deploying pieces of functionality dark). I understand that factory pattern when it comes to NamedSchema, However, UnnamedSchema, while it follows a similar implementation there was nothing in the base class enforcing the use of a factory pattern, which is why this was throwing me off. I'm a bit indifferent about changing it, but NewInstance's accessor is internal (this doesn't exist in Java to my knowledge), which means only the Avro.Main library can create an instance. As far as I can tell moving this to an internal constructor the only place that would need changes are in the Schema.Parse method. So the only way for a user of this library is to call Schema.Parse to create an ArraySchema.
On the other hand, We could make UnnamedSchema follow the factory pattern to enforce it on those that inherit it. That would be a breaking change 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] zcsizmadia edited a comment on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
zcsizmadia edited a comment on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052567195
@KyleSchoonover I totally support the idea if increasing the unit test coverage and I want to ake sure that most likely there is nothing wrong with this particular change.
The theoritical issue I am having is that changing the original library and new unit test for that change are happening at the same time. My immideate concern is that this is passing fine, however the change in the main library is a potential breaking change somewhere else for someb user downstream.
This is probably a good use case IMO to show why changing if conditions and casting and other non-trivial changes should be a low priority. And adding unit tests at the same time can potentially hide breaking changes.
I am not saying that if it is not broken, it should not be changed, however for a mature library, like this, the risk appetite IMO should be low.
--
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] KyleSchoonover commented on a change in pull request #1569: Avro 3421
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on a change in pull request #1569:
URL: https://github.com/apache/avro/pull/1569#discussion_r815085685
##########
File path: lang/csharp/src/apache/main/Schema/ArraySchema.cs
##########
@@ -38,24 +38,27 @@ public class ArraySchema : UnnamedSchema
/// <param name="props">dictionary that provides access to custom properties</param>
/// <param name="names">list of named schemas already parsed</param>
/// <param name="encspace">enclosing namespace for the array schema</param>
- /// <returns></returns>
+ /// <returns>New instance of Array Schema</returns>
internal static ArraySchema NewInstance(JToken jtok, PropertyMap props, SchemaNames names, string encspace)
{
JToken jitem = jtok["items"];
- if (null == jitem) throw new AvroTypeException($"Array does not have 'items' at '{jtok.Path}'");
- var schema = Schema.ParseJson(jitem, names, encspace);
+ if (jitem == null)
+ {
+ throw new AvroTypeException($"Array does not have 'items' at '{jtok.Path}'");
+ }
+
+ Schema schema = Schema.ParseJson(jitem, names, encspace);
return new ArraySchema(schema, props);
}
/// <summary>
- /// Constructor
+ /// Initializes a new instance of the <see cref="ArraySchema"/> class.
/// </summary>
/// <param name="items">schema for the array items type</param>
/// <param name="props">dictionary that provides access to custom properties</param>
private ArraySchema(Schema items, PropertyMap props) : base(Type.Array, props)
{
- if (null == items) throw new ArgumentNullException(nameof(items));
Review comment:
This exception is basically unreachable since the entry point is NewInstance
--
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] KyleSchoonover commented on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052626355
@zcsizmadia I have a Habit of removing bad or dead code, and potentially making things execute in a certain pattern while writing tests. Ideally, I write tests first and then write code. I can break this out into a separate PR, but really I wanted to have a discussion on ArraySchema (really UnnamedSchema).
--
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 #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1052596487
@KyleSchoonover Whoever created ArraySchema was following the "Gang of four" design patterns and more specifically Factory Method.
The idea is to use a static method that does some logic to decide what instance to return.
In the current case it always returns `ArraySchema` but later if needed we could change it to return a specialization of `ArraySchema`.
As I said, usually the factory method has some logic, like the parsing here, but passes "ready to consume data" to the constructor that just does `this.foo = foo;`.
This is what I think the original author did here.
You can change it to plain constructor but what would be the benefit ? Simpler API - maybe!
But we cannot remove the factory method because this will break the old API.
So you will have to copy its code in the new constructor. Not good.
My way of thinking is: if no one complained about it then no one has problem with it. No need to fix something that is not broken.
There are plenty of open tickets in JIRA where users have reported real problems!
--
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] KyleSchoonover edited a comment on pull request #1569: AVRO-3421 Added more test coverage to ArraySchema
Posted by GitBox <gi...@apache.org>.
KyleSchoonover edited a comment on pull request #1569:
URL: https://github.com/apache/avro/pull/1569#issuecomment-1051381652
Just wanted to call this out to @martin-g and @zcsizmadia. Is there a reason the ArraySchema class doesn't just use the following instead of the private constructor and the static NewInstance method?
`internal ArraySchema(JToken jtok, PropertyMap props, SchemaNames names, string encspace) : base(Type.Array, props)
{
}`
With the functionality of both put into this one constructor.
Also the addition of: `private ArraySchema(Schema items, PropertyMap props) : base(Type.Array, props) {}` so that it can't be instantiated without using the internal constructor.
I can put this in a separate PR, but just trying to understand if there was some intention that I don't see.
--
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