[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-11 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r422889031



##
File path: 
sql/core/src/test/resources/sql-tests/results/predicate-functions.sql.out
##
@@ -245,54 +245,54 @@ true
 -- !query
 select to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 04:17:52')
 -- !query schema
-struct<(to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 
04:17:52')):boolean>
+struct<(to_date(2009-07-30 04:17:52) <= to_date(2009-07-30 04:17:52)):boolean>

Review comment:
   Not yet. This PR just makes the `RuntimeReplaceable` -> 
`PrettyAttribute`. I agree with you that the `PrettyAttribute` is not beautiful 
in itself.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-11 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r422882843



##
File path: 
sql/core/src/test/resources/sql-tests/results/predicate-functions.sql.out
##
@@ -245,54 +245,54 @@ true
 -- !query
 select to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 04:17:52')
 -- !query schema
-struct<(to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 
04:17:52')):boolean>
+struct<(to_date(2009-07-30 04:17:52) <= to_date(2009-07-30 04:17:52)):boolean>

Review comment:
   check this
   ```sql
   +-- !query
   +select date_format('2019-10-06', '-MM-dd ')
   +-- !query schema
   +struct
   +-- !query output
   +2019-10-06 Sunday
   ```





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-06 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r420765287



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##
@@ -323,6 +324,18 @@ trait RuntimeReplaceable extends UnaryExpression with 
Unevaluable {
   // two `RuntimeReplaceable` are considered to be semantically equal if their 
"child" expressions
   // are semantically equal.
   override lazy val canonicalized: Expression = child.canonicalized
+
+  /**
+   * Only used to generate SQL representation of this expression.
+   *
+   * Implementations should override this with original parameters
+   */
+  protected def exprsReplaced: Seq[Expression]
+
+  override def sql: String = prettyName + 
exprsReplaced.map(_.sql).mkString("(", ", ", ")")

Review comment:
   SGTM, this way is much better.  We should put the `prettyName` in 
`mkString` though.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-05 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r420022388



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##
@@ -323,6 +324,20 @@ trait RuntimeReplaceable extends UnaryExpression with 
Unevaluable {
   // two `RuntimeReplaceable` are considered to be semantically equal if their 
"child" expressions
   // are semantically equal.
   override lazy val canonicalized: Expression = child.canonicalized
+
+  /**
+   * Only used to generate SQL representation of this expression.
+   *
+   * Implementations should override this with original parameters
+   */
+  protected def exprsReplaced: Seq[Expression]
+
+  override def sql: String = prettyName + 
exprsReplaced.map(_.sql).mkString("(", ", ", ")")
+
+  protected def prettySQL: String =
+prettyName + exprsReplaced.map(toPrettySQL).mkString("(", ", ", ")")
+
+  def asPrettyAttribute(): PrettyAttribute = PrettyAttribute(prettySQL, 
dataType)

Review comment:
   I think it might be better to move this to `util.package`





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-01 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r418520909



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala
##
@@ -142,7 +142,10 @@ package object util extends Logging {
 "`" + name.replace("`", "``") + "`"
   }
 
-  def toPrettySQL(e: Expression): String = usePrettyExpression(e).sql
+  def toPrettySQL(e: Expression): String = e match {
+case r: RuntimeReplaceable => r.prettySQL

Review comment:
   
https://github.com/apache/spark/pull/28420/commits/c42a28360abb3265ae1d0e75b81423120e6a2909#diff-6f4edc80e2cc973e748705e85a6053b4R786-R807
   
   fixed and added some nested tests, thanks, good catch!





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-01 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r418520909



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala
##
@@ -142,7 +142,10 @@ package object util extends Logging {
 "`" + name.replace("`", "``") + "`"
   }
 
-  def toPrettySQL(e: Expression): String = usePrettyExpression(e).sql
+  def toPrettySQL(e: Expression): String = e match {
+case r: RuntimeReplaceable => r.prettySQL

Review comment:
   
https://github.com/apache/spark/pull/28420/files#diff-6f4edc80e2cc973e748705e85a6053b4R788-R807
   
   fixed and added some nested tests, thanks, good catch!





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-05-01 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r418441463



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala
##
@@ -142,7 +142,10 @@ package object util extends Logging {
 "`" + name.replace("`", "``") + "`"
   }
 
-  def toPrettySQL(e: Expression): String = usePrettyExpression(e).sql
+  def toPrettySQL(e: Expression): String = e match {
+case r: RuntimeReplaceable => r.prettySQL

Review comment:
   Ah.. no..





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-04-30 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r418418394



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##
@@ -323,6 +323,20 @@ trait RuntimeReplaceable extends UnaryExpression with 
Unevaluable {
   // two `RuntimeReplaceable` are considered to be semantically equal if their 
"child" expressions
   // are semantically equal.
   override lazy val canonicalized: Expression = child.canonicalized
+
+  override def innerChildren: Seq[Expression] = sys.error("RuntimeReplaceable 
must implement" +
+" innerChildren with the original parameters")
+
+  protected val sqlStrSeparator: String = ", "
+
+  override def sql: String = RuntimeReplaceable.this.prettyName +
+prettyChildren.map(_.sql).mkString("(", sqlStrSeparator, ")")
+
+  protected var prettyChildren: Seq[Expression] = innerChildren

Review comment:
   The `newInstance` seems not robust to make a clone with `Seq[Expression]`
   
   I think we can just add  `def prettySQL: String` in RuntimeReplaceable for  
`utils.package` to call, and override it wherever needed without creating new 
instances.
   
   Seems a better and simpler idea that comes up based on your suggestions, 
WDYT?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

[GitHub] [spark] yaooqinn commented on a change in pull request #28420: [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions

2020-04-30 Thread GitBox 


yaooqinn commented on a change in pull request #28420:
URL: https://github.com/apache/spark/pull/28420#discussion_r418414564



##
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##
@@ -323,6 +323,20 @@ trait RuntimeReplaceable extends UnaryExpression with 
Unevaluable {
   // two `RuntimeReplaceable` are considered to be semantically equal if their 
"child" expressions
   // are semantically equal.
   override lazy val canonicalized: Expression = child.canonicalized
+
+  override def innerChildren: Seq[Expression] = sys.error("RuntimeReplaceable 
must implement" +

Review comment:
   Good Point!





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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