Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan closed pull request #47553: [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter URL: https://github.com/apache/spark/pull/47553 -- 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-unsubscr...@spark.apache.org 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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on PR #47553: URL: https://github.com/apache/spark/pull/47553#issuecomment-2268936300 thanks, merging to master! -- 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-unsubscr...@spark.apache.org 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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1703748683
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
@cloud-fan fixed, made it not an Option
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1702805570
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
Don't have any example at the moment, we can make it non-optional for now
and if we ever figure out there is a need, we will make it optional.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1702030482
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
I don't have an example currently, @davidm-db suggested this so maybe he
could provide more details.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1702022635
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
> It will be None when a future scripting exception does not want to show
the line number
Do you have an example? Let's make sure it's a real use case. We never hide
the Origin in error messages if it's available.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1701970258
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
It will be None when a future scripting exception does not want to show the
line number, if it doesn't make sense in that context. If we removed it
completely as you suggested, exceptions would not be able to choose whether or
not to show the line number, unless we add a flag or something similar in the
place of origin, however the current approach seems more flexible to me.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1701951490
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
If it needs to be optional, then we don't need it as all `TreeNode`
implementations will capture the current Origin.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1701949792
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
If it's never None, shall we change the type to `origin: Origin`?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1701948494
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+origin: Option[Origin],
Review Comment:
when will this be None?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db closed pull request #47537: [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter URL: https://github.com/apache/spark/pull/47537 -- 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-unsubscr...@spark.apache.org 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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698663860
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
Review Comment:
Made it required, but it's still Option[Origin] so None can be passed in.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698664469
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.flatMap(o => o.line.map(l => s"[LINE:$l]
")).getOrElse("")
+prefix + SparkThrowableHelper.getMessage(errorClass, messageParameters)
Review Comment:
Changed to curly braces.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698663860
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
Review Comment:
made it required.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698652131
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -181,10 +183,15 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper
if bl.multipartIdentifier().getText.nonEmpty &&
bl.multipartIdentifier().getText.toLowerCase(Locale.ROOT) !=
el.multipartIdentifier().getText.toLowerCase(Locale.ROOT) =>
-throw SqlScriptingErrors.labelsMismatch(
- bl.multipartIdentifier().getText, el.multipartIdentifier().getText)
+withOrigin(bl) {
+ throw SqlScriptingErrors.labelsMismatch(
+CurrentOrigin.get, bl.multipartIdentifier().getText,
el.multipartIdentifier().getText)
Review Comment:
Sorry, but I dont understand what you mean by this, your suggestion doesn't
compile because bl is BeginLabelContext, not Origin, that's why we included
withOrigin there.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698613479
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
Review Comment:
we did it this way in case there ever was a need to raise exception that's
not tied to a specific line in SQL code. but I guess we can make it required
and pass `None` explicitly in such case, as to enforce people not to forget to
provide `Origin`, if that makes sense?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698613479
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
Review Comment:
we did it this way in case there ever was a need to raise exception that's
not tied to a specific line in SQL code. but I guess we can make it required
and pass `None` explicitly in such case, as to enforce people not to forget to
provide `Origin` if that makes sense?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698605170
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.flatMap(o => o.line.map(l => s"[LINE:$l]
")).getOrElse("")
+prefix + SparkThrowableHelper.getMessage(errorClass, messageParameters)
Review Comment:
The `errorMessage` usually starts with `[ERROR_CLASS]`. Maybe `{LINE:N}
[ERROR_CLASS] message`?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698601968
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
Review Comment:
should we make it a required field?
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
cloud-fan commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698595924
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -181,10 +183,15 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper
if bl.multipartIdentifier().getText.nonEmpty &&
bl.multipartIdentifier().getText.toLowerCase(Locale.ROOT) !=
el.multipartIdentifier().getText.toLowerCase(Locale.ROOT) =>
-throw SqlScriptingErrors.labelsMismatch(
- bl.multipartIdentifier().getText, el.multipartIdentifier().getText)
+withOrigin(bl) {
+ throw SqlScriptingErrors.labelsMismatch(
+CurrentOrigin.get, bl.multipartIdentifier().getText,
el.multipartIdentifier().getText)
Review Comment:
```suggestion
bl, bl.multipartIdentifier().getText,
el.multipartIdentifier().getText)
```
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698401374
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.map(o => o.line.map(l => s"[LINE:$l]
").getOrElse("")).getOrElse("")
Review Comment:
fixed
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698400748
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.flatMap(o => o.line.map(l => s"[LINE:$l]
")).getOrElse("")
+prefix + SparkThrowableHelper.getMessage(errorClass, messageParameters)
Review Comment:
I'm open to suggestions regarding the formatting of the error messages, and
also if we should include the character index of the error, or some other
metadata. Currently the format is
`[LINE:N] errorMessage`
where N is the line number of the error
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698376885
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.map(o => o.line.map(l => s"[LINE:$l]
").getOrElse("")).getOrElse("")
Review Comment:
can be simplified with the use of `flatMap` with `Option`: `origin.flatMap(o
=> o.line.map(l => s"[LINE:$l] ")).getOrElse("")`
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.map(o => o.line.map(l => s"[LINE:$l]
").getOrElse("")).getOrElse("")
Review Comment:
can be simplified with the use of `flatMap` with `Option`:
`origin.flatMap(o => o.line.map(l => s"[LINE:$l] ")).getOrElse("")`
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698376885
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty,
+origin: Option[Origin] = None)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+ with SparkThrowable {
+
+ override def getErrorClass: String = errorClass
+ override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
+}
+
+private object SqlScriptingException {
+
+ private def errorMessageWithLineNumber(
+ origin: Option[Origin],
+ errorClass: String,
+ messageParameters: Map[String, String]): String = {
+val prefix = origin.map(o => o.line.map(l => s"[LINE:$l]
").getOrElse("")).getOrElse("")
Review Comment:
can be simplified with the use of `flatMap` in option: `origin.flatMap(o =>
o.line.map(l => s"[LINE:$l] ")).getOrElse("")`
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on PR #47537: URL: https://github.com/apache/spark/pull/47537#issuecomment-2260300781 @dusantism-db Let's close this PR so people don't get confused with the proper one by accident. -- 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-unsubscr...@spark.apache.org 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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698294903
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+with SparkThrowable {
Review Comment:
fixed
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+origin: Origin,
Review Comment:
fixed
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698278892
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+origin: Origin,
Review Comment:
minor: let's move origin to the end of the parameters and make it an
`Option[Origin]` with default value being `None` - I suppose in future we might
have cases of exceptions that are not tied to the specific line.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47553:
URL: https://github.com/apache/spark/pull/47553#discussion_r1698277277
##
sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingException.scala:
##
@@ -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.sql.exceptions
+
+import scala.jdk.CollectionConverters.MapHasAsJava
+
+import org.apache.spark.{SparkThrowable, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.exceptions.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends Exception(errorMessageWithLineNumber(origin, errorClass,
messageParameters), cause)
+with SparkThrowable {
Review Comment:
nit: `with` should be the same indentation as `extends`
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
dusantism-db commented on PR #47553: URL: https://github.com/apache/spark/pull/47553#issuecomment-2260178444 I'm open to suggestions regarding the formatting of the error messages, and also if we should include the character index of the error, or some other metadata. -- 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-unsubscr...@spark.apache.org 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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697249446
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
Review Comment:
This is fine, but I think it can be further simplified, we can extend
`Exception(message, cause)` the same way that `SparkException` does it, and
implementing a few simple but required override functions. reasoning: there are
properties in `SparkException` that we actually don't need.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697245949
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
Review Comment:
I'm thinking maybe we should leave `SqlScriptingErrors` instead of using
`SqlScriptingException` object - if you look at the `SparkException` as an
example, they use object only for some internal stuff and exceptions, parameter
formatting, etc. while various error classes are used to construct concrete
`SparkExceptions`.
In such case, I would move `SqlScriptingException` to something like
`org.apache.spark.sql.exceptions` - I know it doesn't exist at the moment, but
I think we should create the package since I don't see any more meaningful
place to put the new exception type.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697234609
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
- def labelsMismatch(beginLabel: String, endLabel: String): Throwable = {
-new SparkException(
+ def labelsMismatch(origin: Origin, beginLabel: String, endLabel: String):
Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "LABELS_MISMATCH",
cause = null,
messageParameters = Map("beginLabel" -> beginLabel, "endLabel" ->
endLabel))
}
- def endLabelWithoutBeginLabel(endLabel: String): Throwable = {
-new SparkException(
+ def endLabelWithoutBeginLabel(origin: Origin, endLabel: String): Throwable =
{
+new SqlScriptingException(
+ origin = origin,
errorClass = "END_LABEL_WITHOUT_BEGIN_LABEL",
cause = null,
messageParameters = Map("endLabel" -> endLabel))
}
- def variableDeclarationNotAllowedInScope(varName: String, lineNumber:
String): Throwable = {
-new SparkException(
+ def variableDeclarationNotAllowedInScope(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "INVALID_VARIABLE_DECLARATION.NOT_ALLOWED_IN_SCOPE",
cause = null,
messageParameters = Map("varName" -> varName, "lineNumber" ->
lineNumber))
}
- def variableDeclarationOnlyAtBeginning(varName: String, lineNumber: String):
Throwable = {
-new SparkException(
+ def variableDeclarationOnlyAtBeginning(
+origin: Origin,
Review Comment:
we usually indent parameters for 4 spaces, and lines in the function for 2
spaces, so when you apply all the comments, this function should look like
```
def variableDeclarationOnlyAtBeginning(
origin: Origin,
varName: String,
lineNumber: String): Throwable = {
new SqlScriptingException(...)
}
```
same goes for other functions in this file
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697237880
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
- def labelsMismatch(beginLabel: String, endLabel: String): Throwable = {
-new SparkException(
+ def labelsMismatch(origin: Origin, beginLabel: String, endLabel: String):
Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "LABELS_MISMATCH",
cause = null,
messageParameters = Map("beginLabel" -> beginLabel, "endLabel" ->
endLabel))
}
- def endLabelWithoutBeginLabel(endLabel: String): Throwable = {
-new SparkException(
+ def endLabelWithoutBeginLabel(origin: Origin, endLabel: String): Throwable =
{
+new SqlScriptingException(
+ origin = origin,
errorClass = "END_LABEL_WITHOUT_BEGIN_LABEL",
cause = null,
messageParameters = Map("endLabel" -> endLabel))
}
- def variableDeclarationNotAllowedInScope(varName: String, lineNumber:
String): Throwable = {
-new SparkException(
+ def variableDeclarationNotAllowedInScope(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "INVALID_VARIABLE_DECLARATION.NOT_ALLOWED_IN_SCOPE",
cause = null,
messageParameters = Map("varName" -> varName, "lineNumber" ->
lineNumber))
}
- def variableDeclarationOnlyAtBeginning(varName: String, lineNumber: String):
Throwable = {
-new SparkException(
+ def variableDeclarationOnlyAtBeginning(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "INVALID_VARIABLE_DECLARATION.ONLY_AT_BEGINNING",
cause = null,
messageParameters = Map("varName" -> varName, "lineNumber" ->
lineNumber))
}
+ private def errorMessageWithLineNumber(
+origin: Origin,
+errorClass: String,
+messageParameters: Map[String, String]
+ ): String = {
+val prefix = if (origin.line.isEmpty) "" else s"[LINE:${origin.line.get}] "
Review Comment:
with `Options`, this line can be simplified and written in a more
"functional" manner, something like:
`val lineNumberPrefix = origin.line.map(l => s"[LINE:${l}] ").getOrElse("")`
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697234609
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
- def labelsMismatch(beginLabel: String, endLabel: String): Throwable = {
-new SparkException(
+ def labelsMismatch(origin: Origin, beginLabel: String, endLabel: String):
Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "LABELS_MISMATCH",
cause = null,
messageParameters = Map("beginLabel" -> beginLabel, "endLabel" ->
endLabel))
}
- def endLabelWithoutBeginLabel(endLabel: String): Throwable = {
-new SparkException(
+ def endLabelWithoutBeginLabel(origin: Origin, endLabel: String): Throwable =
{
+new SqlScriptingException(
+ origin = origin,
errorClass = "END_LABEL_WITHOUT_BEGIN_LABEL",
cause = null,
messageParameters = Map("endLabel" -> endLabel))
}
- def variableDeclarationNotAllowedInScope(varName: String, lineNumber:
String): Throwable = {
-new SparkException(
+ def variableDeclarationNotAllowedInScope(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "INVALID_VARIABLE_DECLARATION.NOT_ALLOWED_IN_SCOPE",
cause = null,
messageParameters = Map("varName" -> varName, "lineNumber" ->
lineNumber))
}
- def variableDeclarationOnlyAtBeginning(varName: String, lineNumber: String):
Throwable = {
-new SparkException(
+ def variableDeclarationOnlyAtBeginning(
+origin: Origin,
Review Comment:
we usually indent parameters for 4 spaces, and lines in the function for 2
spaces, so when you apply all the comments, this function should look like
```
def variableDeclarationOnlyAtBeginning(
origin: Origin,
varName: String,
lineNumber: String): Throwable = {
new SqlScriptingException(...)
}
```
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697231118
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
- def labelsMismatch(beginLabel: String, endLabel: String): Throwable = {
-new SparkException(
+ def labelsMismatch(origin: Origin, beginLabel: String, endLabel: String):
Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "LABELS_MISMATCH",
cause = null,
messageParameters = Map("beginLabel" -> beginLabel, "endLabel" ->
endLabel))
}
- def endLabelWithoutBeginLabel(endLabel: String): Throwable = {
-new SparkException(
+ def endLabelWithoutBeginLabel(origin: Origin, endLabel: String): Throwable =
{
+new SqlScriptingException(
+ origin = origin,
errorClass = "END_LABEL_WITHOUT_BEGIN_LABEL",
cause = null,
messageParameters = Map("endLabel" -> endLabel))
}
- def variableDeclarationNotAllowedInScope(varName: String, lineNumber:
String): Throwable = {
-new SparkException(
+ def variableDeclarationNotAllowedInScope(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
Review Comment:
same as in the comment above, usually we leave this part of the code in the
line above: `lineNumber: String): Throwable = {`
there are a few places below with this as well.
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697231118
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
+}
/**
* Object for grouping error messages thrown during parsing/interpreting phase
* of the SQL Scripting Language interpreter.
*/
-private[sql] object SqlScriptingErrors extends QueryErrorsBase {
+private[sql] object SqlScriptingException {
- def labelsMismatch(beginLabel: String, endLabel: String): Throwable = {
-new SparkException(
+ def labelsMismatch(origin: Origin, beginLabel: String, endLabel: String):
Throwable = {
+new SqlScriptingException(
+ origin = origin,
errorClass = "LABELS_MISMATCH",
cause = null,
messageParameters = Map("beginLabel" -> beginLabel, "endLabel" ->
endLabel))
}
- def endLabelWithoutBeginLabel(endLabel: String): Throwable = {
-new SparkException(
+ def endLabelWithoutBeginLabel(origin: Origin, endLabel: String): Throwable =
{
+new SqlScriptingException(
+ origin = origin,
errorClass = "END_LABEL_WITHOUT_BEGIN_LABEL",
cause = null,
messageParameters = Map("endLabel" -> endLabel))
}
- def variableDeclarationNotAllowedInScope(varName: String, lineNumber:
String): Throwable = {
-new SparkException(
+ def variableDeclarationNotAllowedInScope(
+origin: Origin,
+varName: String,
+lineNumber: String
+ ): Throwable = {
Review Comment:
same as in the comment above, usually we leave this part of the code in the
line above: `lineNumber: String): Throwable = {`
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697229129
##
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingException.scala:
##
@@ -17,40 +17,77 @@
package org.apache.spark.sql.errors
-import org.apache.spark.SparkException
+import org.apache.spark.{SparkException, SparkThrowableHelper}
+import org.apache.spark.sql.catalyst.trees.Origin
+import
org.apache.spark.sql.errors.SqlScriptingException.errorMessageWithLineNumber
+
+class SqlScriptingException protected (
+origin: Origin,
+errorClass: String,
+cause: Throwable,
+messageParameters: Map[String, String] = Map.empty)
+ extends SparkException(
+message = errorMessageWithLineNumber(origin, errorClass,
messageParameters),
+errorClass = Option(errorClass),
+cause = cause,
+messageParameters = messageParameters
+ ) {
+
Review Comment:
you can omit {}, it's not needed. also, throughout the code we don't leave
`)` in the separate row, so it's better to do `messageParameters =
messageParameters)` here
--
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-unsubscr...@spark.apache.org
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
Re: [PR] [SPARK-48338][SQL] Improve exceptions thrown from parser/interpreter [spark]
davidm-db commented on code in PR #47537:
URL: https://github.com/apache/spark/pull/47537#discussion_r1697227432
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -181,10 +183,15 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper
if bl.multipartIdentifier().getText.nonEmpty &&
bl.multipartIdentifier().getText.toLowerCase(Locale.ROOT) !=
el.multipartIdentifier().getText.toLowerCase(Locale.ROOT) =>
-throw SqlScriptingErrors.labelsMismatch(
- bl.multipartIdentifier().getText, el.multipartIdentifier().getText)
+withOrigin(bl) {
+ throw SqlScriptingException.labelsMismatch(
+ CurrentOrigin.get, bl.multipartIdentifier().getText,
el.multipartIdentifier().getText)
Review Comment:
nit: this row should be indented for one tab.
--
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-unsubscr...@spark.apache.org
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
