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/03/24 13:17:31 UTC
[GitHub] [avro] anekdoti opened a new pull request #1622: AVRO-3468: Handle default values for logical types
anekdoti opened a new pull request #1622:
URL: https://github.com/apache/avro/pull/1622
Make sure you have checked _all_ steps below.
### Jira
- [x ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO-3468/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3468
### Tests
- [x ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
1. ConvertsDefaultToLogicalType in Avro.Test.Generic.GenericTests
2. DeserializeToLogicalTypeWithDefault in Avro.Test.SpecificTests
### 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
- [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
--
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] martin-g commented on pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1081560432
Close+reopened it to re-trigger the CI checks!
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835416422
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Can you explain? What was the issue with the existing scale?
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835415308
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I had to change the tests (and also TestDecimal) to construct the AvroDecimal with the right scale.
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835444483
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
It turned out that this was a culture problem: on my machine, decimals get parsed with "," as the decimal point by default. Parsing with `CultureInfo.InvariantCulture` solved the problem.
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835429797
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Yes. The AvroDecimal has to have the same scale as the logical type (https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Util/Decimal.cs#L72). However, it seems to be that the scale of the decimal is detected as 0. I guess this requires some additional testing for the conversion between decimal and AvroDecimal - again another topic, but there might be a bug as well now that I think about it. I'll try to check this with another test, but unfortunately I have to discontinue for today soon.
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835346485
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I agree with this - I think `LogicalTypeTests.cs` is the place where for example the proper encoding of decimals as byte sequences should be verified and in particular that it does not change. Currently, it seems that only a proper round trip decimal -> bytes -> decimal is checked and maybe it would make sense to add a test that verifies a specific encoding to ensure compatibility in the future - but I think as well that this is a different topic and 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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835429797
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Yes. The AvroDecimal has to have the same scale as the logical type (https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Util/Decimal.cs#L72). However, it seems to be that the scale of the decimal is detected as 0. I guess this requires some additional testing for the conversion between decimal and AvroDecimal - again another topic, but there might be a bug as well now that I think about it. I'll try to check this with another test (AvroDecimal does not seem to be very well tested altogether), but unfortunately I have to discontinue for today soon.
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835274534
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I just went and started to move the microsecond tests into the logical type tests file in my other PR but gave up quickly :) so lets just use the Generic Tests file for this ;)
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1077761767
@anekdoti Thanks for the PR. This is a graet bug 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.
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835265125
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I see your logic there. My intention was that if someone looks at the unit tests, it provides guidence about what to put phusically into the scheam as default value. Is there any higher level public function which can do what intToUtf8 does? I still feel uneasy about putting a very internal piece of code of the library into the unit test
Btw I checked the code and the internal conversion code of the Decimal class can be modified, and the unit tests still pass. Which is not good and clearly a missing code coverage, but of course it is not in the scope of your PR.
Most likely we need a another PR to extend the code coverage of the Decimal object and cover the ConvertTo... public functions.
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835377983
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
How about this. Leave the tests as they are. I feel that we have identiifed a missing code coverage, which indirectly has on your unit tests. As much as I dislike to piggyback additional changes to PR which are not related, this is kinda connected in some sense. Here is the unit tests to validate the conversions for Decimal and could be added to `LogicalTypeTests.cs`. I think the comments will make it trivial when the code is reviewed, so a different PR might not be needed. If you feel you rather have in in a different PR, that is ok and I will do that and we will sequence merging the PRs properly:
```
[TestCase("0", 0, new byte[] { 0 })]
[TestCase("000000000000000001.01", 2, new byte[] { 101 })]
[TestCase("123456789123456789.56", 2, new byte[] { 0, 171, 84, 169, 143, 129, 101, 36, 108 })]
[TestCase("1234", 0, new byte[] { 4, 210 })]
[TestCase("1234.5", 1, new byte[] { 48, 57 })]
[TestCase("1234.56", 2, new byte[] { 1, 226, 64 })]
[TestCase("-0", 0, new byte[] { 0 })]
[TestCase("-000000000000000001.01", 2, new byte[] { 155 })]
[TestCase("-123456789123456789.56", 2, new byte[] { 255, 84, 171, 86, 112, 126, 154, 219, 148 })]
[TestCase("-1234", 0, new byte[] { 251, 46 })]
[TestCase("-1234.5", 1, new byte[] { 207, 199 })]
[TestCase("-1234.56", 2, new byte[] { 254, 29, 192 })]
// This tests ensures that changes to Decimal.ConvertToBaseValue and ConvertToLogicalValue can be validated (bytes)
public void TestDecimalConvert(string s, int scale, byte[] converted)
{
var schema = (LogicalSchema)Schema.Parse(@$"{{""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4, ""scale"": {scale}}}");
var avroDecimal = new Avro.Util.Decimal();
var decimalVal = (AvroDecimal)decimal.Parse(s);
// TestDecimal tests ConvertToLogicalValue(ConvertToBaseValue(...)) which might hide symmetrical breaking changes in both functions
// The following 2 tests are checking the conversions seperately
// Validate Decimal.ConvertToBaseValue
Assert.AreEqual(converted, avroDecimal.ConvertToBaseValue(decimalVal, schema));
// Validate Decimal.ConvertToLogicalValue
Assert.AreEqual(decimalVal, (AvroDecimal)avroDecimal.ConvertToLogicalValue(converted, schema));
}
```
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835431416
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Fyi, the code snippet I posted above or the original TestDecimal works perfectly locally on my dev machine
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835454906
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Thank you for your support!
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834461751
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
Thank you very much for the comment! I will add further tests. In regards to the two problematic cases you have discovered:
1. The conversion from the base value to the logical value in https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Util/TimestampMicrosecond.cs is responsible for this. If this can be changed at all I would consider this a new PR.
2. Decimal uses either "bytes" or "fixed" as the underlying base schema. Both are represented as a string in the Avro JSON representation, but these strings should not contain a decimal encoding but a binary encoding of a decimal.
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835415308
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I had to change the tests (and also TestDecimal) to construct the AvroDecimal with the right scale. Thank you for all your suggestions!
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1079388509
@zcsizmadia I need a second C# developer to approve C# PRs to merge them.
There is a lot of "action" lately in the C# area, and a lot of disagreement.
In the past I have merged some trivial changes but for more complex ones I'd prefer if at least one C# developer approves the PR of another 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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835157530
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
I have fixed the tests for decimals. Strangely, creating a new AvroDecimal for a TestCaseData instance leads to a stack overflow in NUnit - for this reason, the tests for decimal logical type are separate from the others.
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia edited a comment on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1079395018
@martin-g sounds good and I agree. My only reason I pinged you was to sequence this PR with its dependency to make sure the fix required by this PR is merged before this one to make sure the tests can properly pass. Of course only after you have multiple approvals
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835213902
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I have some concern converting the int value to the decimal implementation in the unit test code. I am worried that we might hide some issues in the unit tests if e.g. we modify Decimal.ConvertToBaseValue with some major breaking change, however the unit test will pass, but the new format is not compatible with the previous one.
Maybe instead of the intToUtf8(...) function you could hard code the actual string representation of the decimal default .
something like "\0" for 0m and "\u0004\u00d2" for 1234m. In this case the unit tests are more rigid and are able to detect changes in the underlying implementation. This might even help with the TestCaseDource stack overflow, however , that might not be the 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] zcsizmadia commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835267523
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I forgot to mention. that there is a `LogicalTypeTests.cs` file, maybe `ConvertsDefaultToLogicalType` unit test belongs there more than in the `GenericTests.cs`
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1079217236
@anekdoti Thanks again for your contribution! @martin-g As soon as https://github.com/apache/avro/pull/1623 merged, this can be merged as well.
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834510189
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
I agree on the assessment of the the 2 issues. I would ignore the decimal unit test failures, however the micros are a bigger issue. I would fix that first and put this PR on hold until that is merged.
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1077865296
@anekdoti Created #1623 to fix the us issues. As soon as that PR is merged, this PR tests should be ok.
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #1622:
URL: https://github.com/apache/avro/pull/1622
--
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] zcsizmadia commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835213902
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I have some concern converting the int value to the decimal implementation in the unit test code. I am worried that we might hide some issues in the unit tests if e.g. we modify a breaking change int Decimal.ConvertToBaseValue code. however the unit test will pass, but the new format is not compatible with the previous one. Maybe instead of the intToUtf8(...) function you could hard code the actual string representation of the decimal default .
something like "\0" for 0m and "\u0004\u00d2" for 1234m. In this case the unit tests are more rigid and are able to detect changes in the underlying implementation. This might even help with the TestCaseDource stack overflow, however , that might not be the 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] zcsizmadia commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835430310
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
No worries. Thanks for your time!
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834444112
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
We need more unit tests here. Something like this:
```
private static IEnumerable<TestCaseData> ConvertsDefaultToLogicalTypeSource = new List<TestCaseData>()
{
new TestCaseData(@"{""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4}", @"""0""", (decimal)0),
new TestCaseData(@"{""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4}", @"""1234""", (decimal)1234),
new TestCaseData(@"{""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4}", @"""1.234""", (decimal)1.234),
new TestCaseData(@"{""type"": ""string"", ""logicalType"": ""uuid""}", @"""00000000-0000-0000-0000-000000000000""", new Guid()),
new TestCaseData(@"{""type"": ""string"", ""logicalType"": ""uuid""}", @"""00000000000000000000000000000000""", new Guid()),
new TestCaseData(@"{""type"": ""string"", ""logicalType"": ""uuid""}", @"""12345678-1234-5678-1234-123456789012""", new Guid("12345678-1234-5678-1234-123456789012")),
new TestCaseData(@"{""type"": ""string"", ""logicalType"": ""uuid""}", @"""12345678123456781234123456789012""", new Guid("12345678-1234-5678-1234-123456789012")),
new TestCaseData(@"{""type"": ""int"", ""logicalType"": ""date""}", "0", DateTime.UnixEpoch),
new TestCaseData(@"{""type"": ""int"", ""logicalType"": ""date""}", "123456", DateTime.UnixEpoch.AddDays(123456)),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""time-micros""}", "0", new TimeSpan()),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""time-micros""}", "123456", new TimeSpan(123456*TimeSpan.TicksPerMillisecond/1000)),
new TestCaseData(@"{""type"": ""int"", ""logicalType"": ""time-millis""}", "0", new TimeSpan()),
new TestCaseData(@"{""type"": ""int"", ""logicalType"": ""time-millis""}", "123456", new TimeSpan(0, 0, 0, 0, 123456)),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""timestamp-micros""}", "0", DateTime.UnixEpoch),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""timestamp-micros""}", "123456", DateTime.UnixEpoch.AddTicks(123456*TimeSpan.TicksPerMillisecond/1000)),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""timestamp-millis""}", "0", DateTime.UnixEpoch),
new TestCaseData(@"{""type"": ""long"", ""logicalType"": ""timestamp-millis""}", "123456", DateTime.UnixEpoch.AddMilliseconds(123456))
};
[TestCaseSource(nameof(ConvertsDefaultToLogicalTypeSource))]
public void ConvertsDefaultToLogicalType(string typeDefinition, string defaultDefinition, object expected)
{
var writerSchemaString = @"{
""type"": ""record"",
""name"": ""Foo"",
""fields"": [
]
}";
var readerSchemaString = $@"{{
""type"": ""record"",
""name"": ""Foo"",
""fields"": [
{{
""name"": ""x"",
""type"": {typeDefinition},
""default"": {defaultDefinition}
}}
]
}}";
var writerSchema = Schema.Parse(writerSchemaString);
Stream stream;
serialize(writerSchemaString,
mkRecord(new object[] { }, (RecordSchema) writerSchema),
out stream,
out _);
var output = deserialize<GenericRecord>(stream, writerSchema, Schema.Parse(readerSchemaString));
Assert.AreEqual(expected, output.GetValue(0));
}
```
You will note that some unit tests do not expect the way I think they should work, so those cases must be discussed.
1. microsec logical types swallow/ignore the actual us values and work only for milliseconds.
2. decimal defaults do not work IMO as expected, however I might have been using them wrong in the test cases
##########
File path: lang/csharp/src/apache/test/Specific/SpecificTests.cs
##########
@@ -440,6 +441,69 @@ public void TestEmbeddedGenerics()
Assert.AreEqual(0, dstRecord.UserMatrix[2].Count);
}
+ private static void serializeGeneric<T>(string writerSchema, T actual, out Stream stream, out Schema ws)
+ {
+ var ms = new MemoryStream();
+ Encoder e = new BinaryEncoder(ms);
+ ws = Schema.Parse(writerSchema);
+ GenericWriter<T> w = new GenericWriter<T>(ws);
+ w.Write(actual, e);
+ ms.Flush();
+ ms.Position = 0;
+ stream = ms;
+ }
+
+
+ [Test]
+ public void DeserializeToLogicalTypeWithDefault()
+ {
+ var writerSchemaString = @"{
+ ""type"": ""record"",
+ ""name"": ""RecordWithOptionalLogicalType"",
+ ""namespace"": ""Avro.Test.Specific.return"",
+ ""fields"": [
+ ]}";
+
+ var writerSchema = Schema.Parse(writerSchemaString);
+
+ Stream stream;
+
+ serializeGeneric(writerSchemaString,
+ mkRecord(new object[] { }, (RecordSchema)writerSchema),
+ out stream,
+ out _);
+
+ RecordWithOptionalLogicalType output = deserialize<RecordWithOptionalLogicalType>(stream, writerSchema, RecordWithOptionalLogicalType._SCHEMA);
+
+ Assert.AreEqual(output.x, new DateTime(1970, 1, 11));
+
+ }
+
+ private static GenericRecord mkRecord(object[] kv, RecordSchema s)
Review comment:
This seems to be the exact replica of the GenericTests.mkRecord() function. Can we make that public and use it, instead uf duplicationg it?
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835226149
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I am not sure about this. I think that what we test here is that a default with the encoding of a decimal (at that time) can be properly read as an instance of a decimal logical type. Therefore I would indeed think that the test should pass even after a breaking change to the encoding.
--
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] anekdoti commented on pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1081586914
Great! Thank you! Is there already a plan to release Avro 1.11.1?
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835450321
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
Roger. the PR is looking good to me. I think your comment about the decimal point `,` vs `.` is very useful. I would add it to that Parse lines to help out the reviewer or/and contributors in the future.
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835049979
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
Sorry for the late reply - thank you very much for your 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] zcsizmadia commented on pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1079395018
@martin-g sounds good and I agree. My only reason I ponged you was to sequence this PR with its dependency to make sure the fix required by this PR is merged before this one to make sure the teats can properly pass. Of course only after you have multiple approvals
--
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 closed pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g closed pull request #1622:
URL: https://github.com/apache/avro/pull/1622
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834465296
##########
File path: lang/csharp/src/apache/test/Specific/SpecificTests.cs
##########
@@ -440,6 +441,69 @@ public void TestEmbeddedGenerics()
Assert.AreEqual(0, dstRecord.UserMatrix[2].Count);
}
+ private static void serializeGeneric<T>(string writerSchema, T actual, out Stream stream, out Schema ws)
+ {
+ var ms = new MemoryStream();
+ Encoder e = new BinaryEncoder(ms);
+ ws = Schema.Parse(writerSchema);
+ GenericWriter<T> w = new GenericWriter<T>(ws);
+ w.Write(actual, e);
+ ms.Flush();
+ ms.Position = 0;
+ stream = ms;
+ }
+
+
+ [Test]
+ public void DeserializeToLogicalTypeWithDefault()
+ {
+ var writerSchemaString = @"{
+ ""type"": ""record"",
+ ""name"": ""RecordWithOptionalLogicalType"",
+ ""namespace"": ""Avro.Test.Specific.return"",
+ ""fields"": [
+ ]}";
+
+ var writerSchema = Schema.Parse(writerSchemaString);
+
+ Stream stream;
+
+ serializeGeneric(writerSchemaString,
+ mkRecord(new object[] { }, (RecordSchema)writerSchema),
+ out stream,
+ out _);
+
+ RecordWithOptionalLogicalType output = deserialize<RecordWithOptionalLogicalType>(stream, writerSchema, RecordWithOptionalLogicalType._SCHEMA);
+
+ Assert.AreEqual(output.x, new DateTime(1970, 1, 11));
+
+ }
+
+ private static GenericRecord mkRecord(object[] kv, RecordSchema s)
Review comment:
Sure!
--
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] anekdoti commented on a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
anekdoti commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834461751
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
Thank you very much for the comment! I will add further tests. In regards to the two problematic cases you have discovered:
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835377983
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
How about this. Leave the tests as they are. I feel that we have identiifed a missing code coverage, which indirectly has an effect on your unit tests. As much as I dislike to piggyback additional changes to PR which are not related, this is kinda connected in some sense. Here is the unit tests to validate the conversions for Decimal and could be added to `LogicalTypeTests.cs`. I think the comments will make it trivial when the code is reviewed, so a different PR might not be needed. If you feel you rather have in in a different PR, that is ok and I will do that and we will sequence merging the PRs properly:
```
[TestCase("0", 0, new byte[] { 0 })]
[TestCase("000000000000000001.01", 2, new byte[] { 101 })]
[TestCase("123456789123456789.56", 2, new byte[] { 0, 171, 84, 169, 143, 129, 101, 36, 108 })]
[TestCase("1234", 0, new byte[] { 4, 210 })]
[TestCase("1234.5", 1, new byte[] { 48, 57 })]
[TestCase("1234.56", 2, new byte[] { 1, 226, 64 })]
[TestCase("-0", 0, new byte[] { 0 })]
[TestCase("-000000000000000001.01", 2, new byte[] { 155 })]
[TestCase("-123456789123456789.56", 2, new byte[] { 255, 84, 171, 86, 112, 126, 154, 219, 148 })]
[TestCase("-1234", 0, new byte[] { 251, 46 })]
[TestCase("-1234.5", 1, new byte[] { 207, 199 })]
[TestCase("-1234.56", 2, new byte[] { 254, 29, 192 })]
// This tests ensures that changes to Decimal.ConvertToBaseValue and ConvertToLogicalValue can be validated (bytes)
public void TestDecimalConvert(string s, int scale, byte[] converted)
{
var schema = (LogicalSchema)Schema.Parse(@$"{{""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4, ""scale"": {scale}}}");
var avroDecimal = new Avro.Util.Decimal();
var decimalVal = (AvroDecimal)decimal.Parse(s);
// TestDecimal tests ConvertToLogicalValue(ConvertToBaseValue(...)) which might hide symmetrical breaking changes in both functions
// The following 2 tests are checking the conversions seperately
// Validate Decimal.ConvertToBaseValue
Assert.AreEqual(converted, avroDecimal.ConvertToBaseValue(decimalVal, schema));
// Validate Decimal.ConvertToLogicalValue
Assert.AreEqual(decimalVal, (AvroDecimal)avroDecimal.ConvertToLogicalValue(converted, schema));
}
```
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r834527927
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -37,6 +37,45 @@ private static void test<T>(string s, T value)
Assert.AreEqual(value, output);
}
+ [Test]
Review comment:
If you dont mind I can create the PR for the microsec fix and as soon as it is merged this can be merged as well. In the meantime you can add all the test cases (except the decimal), and we will know theat the unit test failures will be resolved after the us PR is merged
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1079402003
These two PRs should be easy if you cross approve them! :-)
--
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 a change in pull request #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #1622:
URL: https://github.com/apache/avro/pull/1622#discussion_r835267523
##########
File path: lang/csharp/src/apache/test/Generic/GenericTests.cs
##########
@@ -17,28 +17,64 @@
*/
using System;
using System.IO;
-using System.Linq;
using Avro.IO;
using System.Collections.Generic;
+using System.Text;
using Avro.Generic;
using NUnit.Framework;
+using Decoder = Avro.IO.Decoder;
+using Encoder = Avro.IO.Encoder;
namespace Avro.Test.Generic
{
class GenericTests
{
- private static void test<T>(string s, T value)
+ private static string intToUtf8(int value)
{
- Stream ms;
- Schema ws;
- serialize(s, value, out ms, out ws);
- Schema rs = Schema.Parse(s);
- T output = deserialize<T>(ms, ws, rs);
- Assert.AreEqual(value, output);
+ var decimalLogicalType = new Avro.Util.Decimal();
+ var logicalSchema = (LogicalSchema)
+ Schema.Parse(@"{ ""type"": ""bytes"", ""logicalType"": ""decimal"", ""precision"": 4 }");
+
+ byte[] byteArray = (byte[])decimalLogicalType.ConvertToBaseValue(new AvroDecimal(value), logicalSchema);
Review comment:
I forgot to mention. that there is a LogicalTypeTests.cs file, maybe `ConvertsDefaultToLogicalType` unit test belongs there more than in the `GenericTests.cs`
--
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 #1622: AVRO-3468: Handle default values for logical types
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1622:
URL: https://github.com/apache/avro/pull/1622#issuecomment-1081585701
Thank you for the patience, @anekdoti and @zcsizmadia !
--
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