You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/10/08 10:52:11 UTC
[PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
beliefer opened a new pull request, #43278:
URL: https://github.com/apache/spark/pull/43278
### What changes were proposed in this pull request?
Currently, `StorageLevel` provides `fromString` to get the `StorageLevel`'s instance with its name. So developers or users have to copy the string literal of `StorageLevel`'s name to set or get instance of `StorageLevel`. This issue lead to developers need to manually maintain its consistency. It is easy to make mistakes and reduce development efficiency.
This PR could fix the issue, refer: https://github.com/apache/spark/pull/43259/files#r1349488662
### Why are the changes needed?
Let developers easy to use `StorageLevel`.
### Does this PR introduce _any_ user-facing change?
'No'.
Introduce a new class.
### How was this patch tested?
Exists test cases.
### Was this patch authored or co-authored using generative AI tooling?
'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.
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349686669
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
For example, `StorageLevel.fromString("test")` causes
`java.lang.IllegalArgumentException: No enum constant org.apache.spark.storage.StorageLevelMapper.test`
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349684998
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
The matched `IllegalArgumentException` throws by Java enum, so I convert it to the origin `IllegalArgumentException` throws by `StorageLevel`'s `fromString`.
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349684368
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
Just a question. What is the original exception message? I guess there was no clue for the input?
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43278:
URL: https://github.com/apache/spark/pull/43278#issuecomment-1752299102
@dongjoon-hyun Thank you!
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349684172
##########
common/utils/src/main/java/org/apache/spark/storage/StorageLevelMapper.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.storage;
+
+/**
+ * A mapper class easy to obtain storage levels based on their names.
+ */
+public enum StorageLevelMapper {
+ NONE(StorageLevel.NONE()),
Review Comment:
We use 2-space indentation even in Java.
https://github.com/apache/spark/blob/2d6d09b71e77b362a4c774170e2ca992a31fb1ea/examples/src/main/java/org/apache/spark/examples/JavaSparkPi.java#L31-L37
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43278:
URL: https://github.com/apache/spark/pull/43278#issuecomment-1752153229
Merged to master for Apache Spark 4.0.0.
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349684998
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
The matched `IllegalArgumentException` throws by Java enum, so I convert it to the origin IllegalArgumentException throws by `StorageLevel`'s `fromString`.
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #43278: [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel
URL: https://github.com/apache/spark/pull/43278
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349686669
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
For example, `StorageLevel.fromString("test")` causes
--
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
Re: [PR] [SPARK-45461][CORE][SQL][MLLIB] Introduce a mapper for StorageLevel [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43278:
URL: https://github.com/apache/spark/pull/43278#discussion_r1349685728
##########
common/utils/src/main/scala/org/apache/spark/storage/StorageLevel.scala:
##########
@@ -165,21 +165,13 @@ object StorageLevel {
* Return the StorageLevel object with the specified name.
*/
@DeveloperApi
- def fromString(s: String): StorageLevel = s match {
- case "NONE" => NONE
- case "DISK_ONLY" => DISK_ONLY
- case "DISK_ONLY_2" => DISK_ONLY_2
- case "DISK_ONLY_3" => DISK_ONLY_3
- case "MEMORY_ONLY" => MEMORY_ONLY
- case "MEMORY_ONLY_2" => MEMORY_ONLY_2
- case "MEMORY_ONLY_SER" => MEMORY_ONLY_SER
- case "MEMORY_ONLY_SER_2" => MEMORY_ONLY_SER_2
- case "MEMORY_AND_DISK" => MEMORY_AND_DISK
- case "MEMORY_AND_DISK_2" => MEMORY_AND_DISK_2
- case "MEMORY_AND_DISK_SER" => MEMORY_AND_DISK_SER
- case "MEMORY_AND_DISK_SER_2" => MEMORY_AND_DISK_SER_2
- case "OFF_HEAP" => OFF_HEAP
- case _ => throw new IllegalArgumentException(s"Invalid StorageLevel: $s")
+ def fromString(s: String): StorageLevel = {
+ try {
+ StorageLevelMapper.fromString(s)
+ } catch {
+ case _: IllegalArgumentException =>
Review Comment:
Yes, I know~ The question was what was the original exception message from Java enum.
--
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