Added Array and Map literals for the java scala codebase #50
Conversation
| item: Any => toScala(item) | ||
| } | ||
| case y: java.util.Iterator[_] => | ||
| toScala(y) |
There was a problem hiding this comment.
I'm really sorry about that, I copied an old function from our code base and it is indeed an infinite loop 💥
Fixed it by converting to scala iterator and calling the method toScala to each of its elements.
| case _ => | ||
| x | ||
| } | ||
| } |
There was a problem hiding this comment.
Utility function that recursively convert a java map/array to a scala map/array
| case _ => | ||
| throw new UnsupportedOperationException( | ||
| s"Unsupported datatype by ToSql: ${value.getClass.getName} => $dataType") | ||
| private[analyzer] def toSql(literal: TLiteral): String = { |
There was a problem hiding this comment.
Now takes an instance of TLIteral, it is match against the children classes MapLiteral, ArrayLiteral and Literal. The Literal case contains the original code without change except for BinaryType that is moved inside of ArrayLiteral.
| case mapLiteral: MapLiteral => | ||
| "OBJECT_CONSTRUCT" + mapLiteral.entriesLiterals.flatMap { case (keyLiteral, valueLiteral) => | ||
| Seq(toSql(keyLiteral), toSql(valueLiteral)) | ||
| }.mkString("(", ", ", ")") |
There was a problem hiding this comment.
I use the ARRAY_CONSTRUCT AND OBJECT_CONSTRUCT here and recursively call toSql to populate the arguments of those methods.
| case Seq(_, _*) => Some(MapType(StringType, VariantType)) | ||
| } | ||
| case _ => | ||
| throw ErrorMessage.PLAN_CANNOT_CREATE_LITERAL(value.getClass.getCanonicalName, s"$value") |
There was a problem hiding this comment.
All keys of a Map must be of String type otherwise an exception is thrown. Maybe the exception could be more precise here ?
| case (v, dType) => DataTypeMapper.toSql(v, Option(dType)) | ||
| case (v: Seq[Any], _: ArrayType) => DataTypeMapper.toSql(ArrayLiteral(v)) | ||
| case (v: Map[Any, Any], _: MapType) => DataTypeMapper.toSql(MapLiteral(v)) | ||
| case (v, dType) => DataTypeMapper.toSql(Literal(v, Option(dType))) |
There was a problem hiding this comment.
This part of the code is a bit puzzling me, I think Array and Map are not supported by the Values function, so maybe I should throw something here ?
In any case, the design of the current code is a bit flawed here. Take for example a list of ints that represent DateType, when creating the ArrayLiteral the type is not taken into consideration and is instead infered (so Int). When converting to SQL we will not call the DATE function.
| @@ -36,7 +35,8 @@ private[snowpark] object Literal { | |||
| case t: Timestamp => Literal(DateTimeUtils.javaTimestampToMicros(t), Option(TimestampType)) | |||
| case ld: LocalDate => Literal(DateTimeUtils.localDateToDays(ld), Option(DateType)) | |||
| case d: Date => Literal(DateTimeUtils.javaDateToDays(d), Option(DateType)) | |||
| case a: Array[Byte] => Literal(a, Option(BinaryType)) | |||
| case s: Seq[Any] => ArrayLiteral(s) | |||
| case m: Map[Any, Any] => MapLiteral(m) | |||
There was a problem hiding this comment.
We build the Array and Map Literal here, the data types are infered recursively in the classes themselves.
| item: Any => toScala(item) | ||
| } | ||
| case y: java.util.Iterator[_] => | ||
| toScala(y) |
There was a problem hiding this comment.
I'm really sorry about that, I copied an old function from our code base and it is indeed an infinite loop 💥
Fixed it by converting to scala iterator and calling the method toScala to each of its elements.
|
Is there something else missing to be able to integrate this change? |
|
@sfc-gh-bli any news about this PR? I believe the requested changes were addressed. |
…va-scala in a Coveo fork. SEARCHREL-547
|
@sfc-gh-bli Is there any reason for this PR to be still open? I am noticing, there is now a conflict since the PR is open for long time. Is there anything else missing? Thank you! |
|
@jbergeroncoveo @mpetitclerc |
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-899560: Support Array and Map type as java/scala Literal #47
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Added a trait
TLiteralwith 3 children:ArrayLiteral: Represents an array literalMapLiteral: Represents a map literalLiteral: Represents everything else.The Array and Map Literal holds list of literals corresponding to their elements. Those lists are used to generate the SQL for those types recursively.
Pre-review checklist
(For Snowflake employees)