You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/07/29 03:01:55 UTC

[GitHub] [spark] deshanxiao opened a new pull request, #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

deshanxiao opened a new pull request, #37336:
URL: https://github.com/apache/spark/pull/37336

   ### What changes were proposed in this pull request?
   Today we have two SchemaUtils: SQL SchemaUtils and mllib SchemaUtils. This pr is try to remove SchemaUtils in mllib.
   
   ### Why are the changes needed?
   Two SchemaUtils are often confused for us. MLlib SchemaUtils add a TODO flag and now we can do it.
   
   
   ### Does this PR introduce _any_ user-facing change?
   No.
   
   
   ### How was this patch tested?
   exist UT
   


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] deshanxiao commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
deshanxiao commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r934075866


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   > I don't have a strong preference, but it seems a bit weird to define methods in SQL but only call them in MLlib.
   
   Yes, but it seems more weird  to have a SchemaUtils in the MLiib too. Do you have any good advice?



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] cloud-fan commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r934109571


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   Maybe we can rename the `SchemaUtils` in MLlib so that the name is more ML specific?



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] zhengruifeng commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r934077297


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   -1
   
   also feel weird since those utils are only dedicated for ML.
   
   



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] deshanxiao commented on pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
deshanxiao commented on PR #37336:
URL: https://github.com/apache/spark/pull/37336#issuecomment-1201931299

   Very happy to receive these suggestions. Please let me know if anyone have any questions. Will close this PR.


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] zhengruifeng commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r934118794


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   I don't feel strongly about it. WDYT @srowen @WeichenXu123 ?



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] cloud-fan commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r933171073


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   I don't have a strong preference, but it seems a bit weird to define methods in SQL but only call them in MLlib.



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] deshanxiao closed pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
deshanxiao closed pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL
URL: https://github.com/apache/spark/pull/37336


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] srowen commented on a diff in pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #37336:
URL: https://github.com/apache/spark/pull/37336#discussion_r934463178


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala:
##########
@@ -297,4 +296,117 @@ private[spark] object SchemaUtils {
       .replaceAll("\u000B", "\\\\v")
       .replaceAll("\u0007", "\\\\a")
   }
+
+  /**
+   * Check whether the given schema contains a column of the required data type.

Review Comment:
   Overall I'm not sure it simplifies things, just moves them around. I'd be neutral on this



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] deshanxiao commented on pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
deshanxiao commented on PR #37336:
URL: https://github.com/apache/spark/pull/37336#issuecomment-1198997109

   > Could you make the GitHub Action CI happy, @deshanxiao ?
   
   Sure, seem that all checks have passed.


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] deshanxiao commented on pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
deshanxiao commented on PR #37336:
URL: https://github.com/apache/spark/pull/37336#issuecomment-1198839786

   CC @gengliangwang  @dongjoon-hyun @cloud-fan 


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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

[GitHub] [spark] AmplabJenkins commented on pull request #37336: [SPARK-39916][SQL][MLLIB][REFACTOR] Merge ml SchemaUtils to SQL

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37336:
URL: https://github.com/apache/spark/pull/37336#issuecomment-1199076410

   Can one of the admins verify this patch?


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

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


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