zio / zio-jdbc

A small, idiomatic ZIO interface to JDBC.
Apache License 2.0
82 stars 64 forks source link

fromSchema ARRAY mapping corrected (using Chunk) #159

Closed peterlopen closed 11 months ago

peterlopen commented 11 months ago

hi, I found out original ARRAY mapping was not working, this should fix it. in schema ARRAY should be represented as Chunk. also mapping for BIT should be more compatible (based on docs, works for postgres). any suggestion to improve code welcome, wrapping everything in Some is not ideal, could not figure out how to do it using partial function and case statements. thank you.

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

guizmaii commented 11 months ago

@peterlopen To do it with partial functions you need to do somethink like this:

  private[jdbc] def createDynamicDecoder(schema: Schema[_], meta: ResultSetMetaData): ResultSet => DynamicValue =
    resultSet => {
      val remapName   = createRemapTable(schema)
      var columnIndex = 1
      var listMap     = ListMap.empty[String, DynamicValue]

      while (columnIndex <= meta.getColumnCount()) {
        val name                = remapName(meta.getColumnName(columnIndex))
        val sqlType             = meta.getColumnType(columnIndex)
        val value: DynamicValue =
          (mapPrimitiveType(resultSet, columnIndex) orElse mapComplexType(resultSet, columnIndex))
            .applyOrElse(
              sqlType,
              _ =>
                throw new SQLException(
                  s"Unsupported SQL type ${sqlType} when attempting to decode result set from a ZIO Schema ${schema}"
                )
            )

        listMap = listMap.updated(name, value)

        columnIndex += 1
      }

      DynamicValue.Record(TypeId.Structural, listMap)
    }

  private def mapPrimitiveType(
    resultSet: ResultSet,
    columnIndex: Int
  ): PartialFunction[Int, DynamicValue] = {
    case SqlTypes.BIGINT =>
      val bigInt = resultSet.getBigDecimal(columnIndex).toBigInteger()

      DynamicValue.Primitive(bigInt, StandardType.BigIntegerType)

   ...

    case SqlTypes.VARCHAR =>
      val string = resultSet.getString(columnIndex)

      DynamicValue.Primitive(string, StandardType.StringType)
  }

  private def mapComplexType(resultSet: ResultSet, columnIndex: Int): PartialFunction[Int, DynamicValue] = {
    case SqlTypes.ARRAY =>
      val arrayRs  = resultSet.getArray(columnIndex).getResultSet
      val metaData = arrayRs.getMetaData
      val data     = mutable.ArrayBuffer.empty[DynamicValue]
      val add      = mapPrimitiveType(arrayRs, 2) andThen (el => { data += el; () })

      while (arrayRs.next())
        add.applyOrElse(metaData.getColumnType(2), _ => ())

      DynamicValue.Sequence(Chunk.fromArray(data.toArray))
  }
peterlopen commented 11 months ago

hi @guizmaii , thank you for comments, will look into it.

peterlopen commented 11 months ago

hi @guizmaii ,

thanks for helping me with code. I am wondering - in my partial function attempt, I got match error exception from mapPrimitiveType when SqlTypes.ARRAY was mapped. I was not using orElse with mapComplexType, so that is the reason probably. my solution was to usy Try and Try.toOption and then Option.getOrElse which defeat all idea of using PartialFunction. however, in your approach orElse is handling this problem. I tried to debug how it figures out mapPrimitiveType is not defined for SqlTypes.ARRAY, but could not find it. there is no exception and this code from OrElse implementation

    override def applyOrElse[A1 <: A, B1 >: B](x: A1, default: A1 => B1): B1 = {
      val z = f1.applyOrElse(x, checkFallback[B])
      if (!fallbackOccurred(z)) z else f2.applyOrElse(x, default)
    }

just magically figures out that f2 needs to be called. Please do you know how it is handled internally? Thank you. regards, peter

guizmaii commented 11 months ago

Please do you know how it is handled internally?

I don't know, sorry but indeed, as I mentioned here https://github.com/zio/zio-jdbc/pull/159#discussion_r1366609541 your current code is unsafe and should probably adopt the way I gave you, using applyOrElse

guizmaii commented 11 months ago

@peterlopen Why closing?

peterlopen commented 11 months ago

@guizmaii I found two other issues with using arrays with zio-jdbc, so I gave up on this approach. for the record: 1) incorrect handling of empty arrays, also JDBC approach is to use just one '?' placeholder for array value: https://github.com/zio/zio-jdbc/blob/95cf951aba6ec7a1aa419a6c32c14a2683e2b82f/core/src/main/scala/zio/jdbc/ZConnection.scala#L55 2) follows approach from 1), although in JDBC PreparedStatement.setArray should be used: https://github.com/zio/zio-jdbc/blob/95cf951aba6ec7a1aa419a6c32c14a2683e2b82f/core/src/main/scala/zio/jdbc/SqlFragment.scala#L356

I do not mind merging it, but without fixing other issues it still be incomplete.