Skip to content

Conversation

@ghik
Copy link
Contributor

@ghik ghik commented Apr 3, 2018

See updated GenCodec docs and ObjectInput scaladocs for more details.

TLDR: instead of preserving field order, ObjectInput may alternatively override peekField and provide random field access by field name. This will allow formats using hashtable-based object representation (e.g. native JS representation) to be fully compatible with GenCodec - this currently only affects flat sealed hierarchy encoding and possible custom-written codecs.

@ghik ghik requested review from Starzu and ddworak April 3, 2018 10:43
}
}

def read(): T = if (input.hasNext) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: can we have if and else at the same level of indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a price of one more level of indentation, but whatever.

}
def readCase(caseNameField: FieldInput): T = {
val caseName = readCaseName(caseNameField)
caseIndexByName(caseName) match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 unrelated questions about caseIndexByName:

  1. Why can't we just use .indexOf(element, index)?
  2. Does it make sense to use a more lookup-friendly data structure?

Copy link
Contributor Author

@ghik ghik Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. .indexOf(element, index) would put the underlying Array through implicit conversion to WrappedArray. I wanted to be as raw as possible.

  2. I'm deliberately using Array and linear search here (and also in FieldValues) because case classes tend to have small number of fields and sealed hierarchies tend to have small number of case classes. Using a map would probably introduce more overhead. Also, note that if we wanted to improve on time complexity then the most natural thing would be a binary search, not using a map.

bound to any format. It only depends on the fact that this format is capable of serializing following types and structures:
* integer numbers up to at least 64-bit precision (i.e. `Long`)
* decimal numbers up to at least 64-bit precision (i.e. `Double`)
* `Char`s, `String`s, `Boolean`s and `null`s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have both .nextField and .peekField, we need an example more complex than a single field class and less automatically handled than a case class. I've seen a lot of people struggle with creating 2-field class codec and it's just getting harder.

More generally, should I peek or should I next? It appears to me that GenCodec won't exactly be completely Input/Output indepedent.

Copy link
Contributor Author

@ghik ghik Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to write a custom codec that creates an object with two-three fields then the way to go is to implement apply/unapply in companion object or use fromApplyUnapplyProvider. This is now very clearly recommended in the docs.

You should almost never need to implement an object codec manually. If you really need to do it then the backend-independent way of doing it would be:

class Klass(val int: Int, val str: String)
object Klass {
  private def getField(input: ObjectInput, name: String):
    input.peekField(name).orElse(input.nextField().opt).filter(_.fieldName == name)
      .getOrElse(throw new ReadFailure(s"expected field $name")

  implicit val codec: GenCodec[Klass] = GenCodec.createNullableObject(
    input => {
      val int = getField(input, "int").readInt()
      val str = getField(input, "str").readString()
      new Klass(int, str)
    },
    (klass, output) => {
      output.writeField("int").writeInt(klass.int)
      output.writeField("str").writeString(klass.str)
    }
  )
}

but really, this should be an absolute last resort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input and Output interfaces are generally optimized for ease of implementation and performance rather than ease of use.

}

final def readObject(input: ObjectInput): T = {
val oooFields = new FieldValues(oooFieldNames, oooDeps, typeRepr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: are there aaa fields as well? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nooo

def readCase(caseNameField: FieldInput): T = {
val caseName = readCaseName(caseNameField)
caseIndexByName(caseName) match {
case -1 => unknownCase(caseName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These barbaric checks could be avoided if caseIndexByName would just return Opt[GenCodec[_: T]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I can't similarly change caseIndexByValue because in writeObject I need to access both caseNames and caseDeps arrays. So I have to work with indices anyway.

@ghik ghik merged commit a342a55 into master Apr 9, 2018
@ghik ghik deleted the generalizations branch April 9, 2018 10:15
@ghik ghik restored the generalizations branch January 11, 2019 12:49
@ghik ghik deleted the generalizations branch January 11, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants