[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185794875
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -112,6 +112,102 @@ object JavaCode {
   def isNullExpression(code: String): SimpleExprValue = {
 expression(code, BooleanType)
   }
+
+  def block(code: String): Block = {
+CodeBlock(codeParts = Seq(code), exprValues = Seq.empty)
+  }
+}
+
+/**
+ * A block of java code which involves some expressions represented by 
`ExprValue`.
+ */
+trait Block extends JavaCode {
+  def exprValues: Seq[Any]
+
+  // This will be called during string interpolation.
+  override def toString: String = _marginChar match {
+case Some(c) => code.stripMargin(c)
+case _ => code
+  }
+
+  var _marginChar: Option[Char] = None
+
+  def stripMargin(c: Char): this.type = {
+_marginChar = Some(c)
+this
+  }
+
+  def stripMargin: this.type = {
+_marginChar = Some('|')
+this
+  }
+
+  def + (other: Block): Block
+}
+
+object Block {
+  implicit def blockToString(block: Block): String = block.toString
+
+  implicit def blocksToBlock(blocks: Seq[Block]): Block = Blocks(blocks)
+
+  implicit class BlockHelper(val sc: StringContext) extends AnyVal {
+def code(args: Any*): Block = {
--- End diff --

Ok.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185794699
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -120,13 +216,15 @@ object JavaCode {
 trait ExprValue extends JavaCode {
   def javaType: Class[_]
   def isPrimitive: Boolean = javaType.isPrimitive
+
+  // This will be called during string interpolation.
+  override def toString: String = ExprValue.exprValueToString(this)
 }
 
 object ExprValue {
-  implicit def exprValueToString(exprValue: ExprValue): String = 
exprValue.toString
+  implicit def exprValueToString(exprValue: ExprValue): String = 
exprValue.code
--- End diff --

Because `toString` will call `exprValueToString`. We should have only one 
place to convert an `ExprValue` to string.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread mgaido91
Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185761453
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -120,13 +216,15 @@ object JavaCode {
 trait ExprValue extends JavaCode {
   def javaType: Class[_]
   def isPrimitive: Boolean = javaType.isPrimitive
+
+  // This will be called during string interpolation.
+  override def toString: String = ExprValue.exprValueToString(this)
 }
 
 object ExprValue {
-  implicit def exprValueToString(exprValue: ExprValue): String = 
exprValue.toString
+  implicit def exprValueToString(exprValue: ExprValue): String = 
exprValue.code
--- End diff --

why can't we use `toString`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread mgaido91
Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185761604
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -112,6 +112,102 @@ object JavaCode {
   def isNullExpression(code: String): SimpleExprValue = {
 expression(code, BooleanType)
   }
+
+  def block(code: String): Block = {
+CodeBlock(codeParts = Seq(code), exprValues = Seq.empty)
+  }
+}
+
+/**
+ * A block of java code which involves some expressions represented by 
`ExprValue`.
+ */
+trait Block extends JavaCode {
+  def exprValues: Seq[Any]
+
+  // This will be called during string interpolation.
+  override def toString: String = _marginChar match {
+case Some(c) => code.stripMargin(c)
+case _ => code
+  }
+
+  var _marginChar: Option[Char] = None
+
+  def stripMargin(c: Char): this.type = {
+_marginChar = Some(c)
+this
+  }
+
+  def stripMargin: this.type = {
+_marginChar = Some('|')
+this
+  }
+
+  def + (other: Block): Block
+}
+
+object Block {
+  implicit def blockToString(block: Block): String = block.toString
+
+  implicit def blocksToBlock(blocks: Seq[Block]): Block = Blocks(blocks)
+
+  implicit class BlockHelper(val sc: StringContext) extends AnyVal {
+def code(args: Any*): Block = {
+  if (sc.parts.length == 0) {
+EmptyBlock
+  } else {
+args.foreach {
+  case _: ExprValue => true
--- End diff --

why `true`? Can't we just do nothing after the `=>`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread mgaido91
Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185763081
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -112,6 +112,102 @@ object JavaCode {
   def isNullExpression(code: String): SimpleExprValue = {
 expression(code, BooleanType)
   }
+
+  def block(code: String): Block = {
+CodeBlock(codeParts = Seq(code), exprValues = Seq.empty)
+  }
+}
+
+/**
+ * A block of java code which involves some expressions represented by 
`ExprValue`.
+ */
+trait Block extends JavaCode {
+  def exprValues: Seq[Any]
+
+  // This will be called during string interpolation.
+  override def toString: String = _marginChar match {
+case Some(c) => code.stripMargin(c)
+case _ => code
+  }
+
+  var _marginChar: Option[Char] = None
+
+  def stripMargin(c: Char): this.type = {
+_marginChar = Some(c)
+this
+  }
+
+  def stripMargin: this.type = {
+_marginChar = Some('|')
+this
+  }
+
+  def + (other: Block): Block
+}
+
+object Block {
+  implicit def blockToString(block: Block): String = block.toString
+
+  implicit def blocksToBlock(blocks: Seq[Block]): Block = Blocks(blocks)
+
+  implicit class BlockHelper(val sc: StringContext) extends AnyVal {
+def code(args: Any*): Block = {
+  if (sc.parts.length == 0) {
+EmptyBlock
+  } else {
+args.foreach {
+  case _: ExprValue => true
+  case _: Int | _: Long | _: Float | _: Double | _: String => true
+  case _: Block => true
+  case other => throw new IllegalArgumentException(
+s"Can not interpolate ${other.getClass} into code block.")
+}
+CodeBlock(sc.parts, args)
+  }
+}
+  }
+}
+
+/**
+ * A block of java code.
+ */
+case class CodeBlock(codeParts: Seq[String], exprValues: Seq[Any]) extends 
Block {
+  override def code: String = {
+val strings = codeParts.iterator
+val expressions = exprValues.iterator
+var buf = new StringBuffer(strings.next)
+while (strings.hasNext) {
+  if (expressions.hasNext) {
--- End diff --

this is not needed (especially if we add the `checkLengths`), because if it 
is not true we are in a bad situation


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21193: [SPARK-24121][SQL][WIP] Add API for handling expr...

2018-05-03 Thread mgaido91
Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/21193#discussion_r185762835
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala
 ---
@@ -112,6 +112,102 @@ object JavaCode {
   def isNullExpression(code: String): SimpleExprValue = {
 expression(code, BooleanType)
   }
+
+  def block(code: String): Block = {
+CodeBlock(codeParts = Seq(code), exprValues = Seq.empty)
+  }
+}
+
+/**
+ * A block of java code which involves some expressions represented by 
`ExprValue`.
+ */
+trait Block extends JavaCode {
+  def exprValues: Seq[Any]
+
+  // This will be called during string interpolation.
+  override def toString: String = _marginChar match {
+case Some(c) => code.stripMargin(c)
+case _ => code
+  }
+
+  var _marginChar: Option[Char] = None
+
+  def stripMargin(c: Char): this.type = {
+_marginChar = Some(c)
+this
+  }
+
+  def stripMargin: this.type = {
+_marginChar = Some('|')
+this
+  }
+
+  def + (other: Block): Block
+}
+
+object Block {
+  implicit def blockToString(block: Block): String = block.toString
+
+  implicit def blocksToBlock(blocks: Seq[Block]): Block = Blocks(blocks)
+
+  implicit class BlockHelper(val sc: StringContext) extends AnyVal {
+def code(args: Any*): Block = {
--- End diff --

chat about adding `checkLengths(args)`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org