Add joda-time and ByteBuffer support in avro #740

RustedBones · 2023-05-04T11:55:04Z

avro module part of #708 and #726

Added val afDuration: AvroField[(Long, Long, Long)] definition (not implicit) for matching duration logical type spec. Part of #255

codecov · 2023-05-04T14:10:50Z

Codecov Report

Merging #740 (f0114e0) into main (67eb61f) will increase coverage by 0.07%.
The diff coverage is 95.74%.

@@            Coverage Diff             @@
##             main     #740      +/-   ##
==========================================
+ Coverage   94.96%   95.04%   +0.07%     
==========================================
  Files          52       52              
  Lines        1789     1856      +67     
  Branches      168      168              
==========================================
+ Hits         1699     1764      +65     
- Misses         90       92       +2

Impacted Files	Coverage Δ
...rc/main/scala/magnolify/avro/logical/package.scala	`93.82% <94.66%> (+0.49%)`	⬆️
avro/src/main/scala/magnolify/avro/AvroType.scala	`97.10% <100.00%> (+1.19%)`	⬆️

RustedBones · 2023-05-04T16:16:36Z

avro/src/main/scala/magnolify/avro/AvroType.scala

-    override def from(v: ByteBuffer)(cm: CaseMapper): Array[Byte] =
-      ju.Arrays.copyOfRange(v.array(), v.position(), v.limit())
-    override def to(v: Array[Byte])(cm: CaseMapper): ByteBuffer = ByteBuffer.wrap(v)


Here we have a non symmetric definition:

in the from we copy the data

in the to we only wrap, meaning a change to the converted avro object will modify the scala array.

interesting--yeah, I can't see why we would need to do an array-copy here, your change makes sense to me.

RustedBones · 2023-05-04T16:17:12Z

avro/src/main/scala/magnolify/avro/AvroType.scala

  }
+  implicit val afBytes: AvroField[Array[Byte]] = from[ByteBuffer](_.array())(ByteBuffer.wrap)


Changed the behavior to pass the same mutable array from avro to scala

clairemcginty · 2023-05-10T13:16:59Z

avro/src/main/scala/magnolify/avro/AvroType.scala

-    override def from(v: ByteBuffer)(cm: CaseMapper): Array[Byte] =
-      ju.Arrays.copyOfRange(v.array(), v.position(), v.limit())
-    override def to(v: Array[Byte])(cm: CaseMapper): ByteBuffer = ByteBuffer.wrap(v)


interesting--yeah, I can't see why we would need to do an array-copy here, your change makes sense to me.

clairemcginty · 2023-05-10T13:19:16Z

avro/src/main/scala/magnolify/avro/AvroType.scala

  implicit val afDate: AvroField[LocalDate] =
    logicalType[Int](LogicalTypes.date())(x => LocalDate.ofEpochDay(x.toLong))(_.toEpochDay.toInt)
+  private val EpochJodaDate = new org.joda.time.LocalDate(1970, 1, 1)


can this be lazy?

clairemcginty · 2023-05-10T13:26:36Z

avro/src/main/scala/magnolify/avro/AvroType.scala

@@ -16,7 +16,7 @@

 package magnolify.avro

-import java.nio.ByteBuffer
+import java.nio.{ByteBuffer, ByteOrder}
 import java.time._


Maybe we could import joda as import org.joda.{time => joda} so that the Joda APIs could simply refer to joda.LocalDate?

clairemcginty · 2023-05-10T13:31:57Z

avro/src/main/scala/magnolify/avro/AvroType.scala

+  // A duration logical type annotates Avro fixed type of size 12, which stores three little-endian unsigned integers
+  // that represent durations at different granularities of time.
+  // The first stores a number in months, the second stores a number in days, and the third stores a number in milliseconds.
+  val afDuration: AvroField[(Long, Long, Long)] =


Could we use java.time.Duration for this? It supports a nanosecond granularity which we could use for encoding in the ByteBuffer

Duration is not made to hold month but Period can, so we could define as (Period, Duration).
I was unsure though where to store the days, as both can hold the value. I think we can split like this:

Period: months + days

Duration: milliseconds

Actually, Period is defined as (years: Int, months: Int, days: Int), so we can have an overflow when converting from avro, as the specification tells the value are unsinged integers (handled as long)

ok, got it. This is probably the least painful solution, then -- can you add documentation (both on how to import it, and on what it represents) to https://github.com/spotify/magnolify/blob/main/docs/avro.md + https://github.com/spotify/magnolify/blob/main/docs/mapping.md ?

shnapz · 2023-05-10T15:19:17Z

avro/src/main/scala/magnolify/avro/AvroType.scala

@@ -196,14 +196,14 @@ object AvroField {
  implicit val afLong: AvroField[Long] = id[Long](Schema.Type.LONG)
  implicit val afFloat: AvroField[Float] = id[Float](Schema.Type.FLOAT)
  implicit val afDouble: AvroField[Double] = id[Double](Schema.Type.DOUBLE)
-  implicit val afBytes: AvroField[Array[Byte]] = new Aux[Array[Byte], ByteBuffer, ByteBuffer] {
+  implicit val afByteBuffer: AvroField[ByteBuffer] = new Aux[ByteBuffer, ByteBuffer, ByteBuffer] {


looks like it could be declared as id[ByteBuffer](Schema.Type. BYTES), but id needs to have an override to accommodate d.array()

shnapz · 2023-05-10T15:27:29Z

avro/src/test/scala/magnolify/avro/AvroTypeSuite.scala

+
+    implicit val afAvroDuration: AvroField[AvroDuration] = AvroField.from[(Long, Long, Long)] {
+      case (months, days, millis) => AvroDuration(months, days, millis)
+    }(d => (d.months, d.days, d.millis))(AvroField.afDuration)


We either need explain how to use AvroField.afDuration in docs, or introduce AvroDuration in magnolify. It is concerning that it won't be useful to users

I'm not for exposing a custom AvroDuration class. IHMO, the avro java lib does not expose the duration logical type because there does not exists a proper java data structure to handle the value as defined in the spec.

In the end afDuration is not different from any other type, so users should not be lost.

What I was thinking is that it might be not obvious for a user how to discover afDuration

We can include that in #612 . We'll need to create a proper doc for all modules

* Add joda-time and ByteBuffer support in avro * Move joda-time to common * Add avro duration logical-type * handle unsigned integers values properly * Add all logical types tests * FIx constant naming * Fix default bytes behavior * Use deepEquals for DefaultBytes test * Import org.joda.time package

RustedBones added 3 commits May 4, 2023 13:53

Add joda-time and ByteBuffer support in avro

bb41853

Move joda-time to common

9ed9065

Add avro duration logical-type

71abf92

RustedBones added 4 commits May 4, 2023 16:34

handle unsigned integers values properly

5f23eb7

Add all logical types tests

ae2d53c

FIx constant naming

4ba749c

Fix default bytes behavior

fdc9adc

RustedBones commented May 4, 2023

View reviewed changes

Use deepEquals for DefaultBytes test

2ff7e54

clairemcginty reviewed May 10, 2023

View reviewed changes

shnapz reviewed May 10, 2023

View reviewed changes

Import org.joda.time package

f0114e0

shnapz approved these changes May 11, 2023

View reviewed changes

RustedBones merged commit 4754196 into main May 12, 2023

RustedBones deleted the avro-types branch May 12, 2023 07:28

gafiatulin mentioned this pull request Jan 13, 2024

Regression in bytes read safety in avro #899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add joda-time and ByteBuffer support in avro #740

Add joda-time and ByteBuffer support in avro #740

RustedBones commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading

RustedBones May 4, 2023

clairemcginty May 10, 2023

RustedBones May 4, 2023

clairemcginty May 10, 2023

clairemcginty May 10, 2023

clairemcginty May 10, 2023

clairemcginty May 10, 2023

RustedBones May 11, 2023

RustedBones May 11, 2023

clairemcginty May 11, 2023

shnapz May 10, 2023

shnapz May 10, 2023

RustedBones May 11, 2023

shnapz May 11, 2023

RustedBones May 12, 2023

		}
		implicit val afBytes: AvroField[Array[Byte]] = from[ByteBuffer](_.array())(ByteBuffer.wrap)

Add joda-time and ByteBuffer support in avro #740

Add joda-time and ByteBuffer support in avro #740

Conversation

RustedBones commented May 4, 2023 • edited Loading

codecov bot commented May 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RustedBones commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading