LuciferYang commented on code in PR #58225:
URL: https://github.com/apache/spark/pull/58225#discussion_r3890227238
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -11723,7 +11723,7 @@
},
"_LEGACY_ERROR_TEMP_3070" : {
"message" : [
- "<internalName> is a reserved column name that cannot be read in
combination with <colName> column."
+ "Unrecognized file metadata field: <field>"
Review Comment:
This class of bug is statically checkable: compare the placeholders in
`error-conditions.json` against the keys of the literal `Map(...)` next to each
`errorClass = "X"`. I ran that over main code and it hits exactly the sites
this PR changes plus the two in the comment above; the only other hits are the
allowlisted `CAST_*` conditions passing `ansiConfig`. Landing it as a dev
script or a test would remove the manual sweep next time.
What not to do is extend `checkIfUnique` in `SparkThrowableSuite.scala:175`
to legacy conditions. Seven template groups repeat among legacy conditions.
Only three are purely legacy (0060/0062/0063, 3113/3114, 3152/3155); the other
four also collide with named conditions (the empty template, `<message>`,
`<msg>`, `<errorMessage>`), so that check would fail immediately.
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:
##########
@@ -141,6 +141,21 @@ class QueryExecutionErrorsSuite
(df1, df2)
}
+ test("SPARK-58945: invalid file extension reports invalidValue") {
+ withTempDir { dir =>
+ val path = new File(dir, "data").getCanonicalPath
+ checkError(
+ exception = intercept[SparkIllegalArgumentException] {
+ spark.range(1).write.option("extension", "12").csv(path)
Review Comment:
`CSVOptions.scala:127` reads `ext.size != 3 && !ext.forall(_.isLetter)`, so
both conditions must hold before it rejects anything. All-letter values like
`ab` and `toolong`, and three-character values like `123` and `a/b`, all pass,
while the message says the extension is `limited to exactly 3 letters`. The
intent is clearly `||`. The value is concatenated into the output file name
(`CSVFileFormat.scala:91`), so `a/b` puts a path separator there.
That line is not part of this PR, and `12` is just one of the values the
current condition does reject. Flipping `&&` to `||` and adding an `ab` case is
the cheap fix. If that is out of scope, a separate ticket plus a comment in the
test saying which shapes are actually covered would do.
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:
##########
@@ -141,6 +141,21 @@ class QueryExecutionErrorsSuite
(df1, df2)
}
+ test("SPARK-58945: invalid file extension reports invalidValue") {
+ withTempDir { dir =>
+ val path = new File(dir, "data").getCanonicalPath
+ checkError(
+ exception = intercept[SparkIllegalArgumentException] {
+ spark.range(1).write.option("extension", "12").csv(path)
+ },
+ condition = "INVALID_PARAMETER_VALUE.EXTENSION",
+ parameters = Map(
+ "functionName" -> "`extension`",
Review Comment:
The rendered message is ``The value of parameter(s) `extension` in
`extension` is invalid: ...``. The slot after `in` is meant to hold a function
name, and it repeats the option name because `CSVOptions.scala:128` passes the
`EXTENSION` constant as `functionName`.
Before this fix the path always degraded to `INTERNAL_ERROR`, so this PR is
what makes the message visible for the first time. That makes it the right
moment to pass the data source name (`"csv"`) as the first argument at
`CSVOptions.scala:128`, with a one-word change in the test. Otherwise the new
assertion locks the wrong wording into a regression test.
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:
##########
@@ -141,6 +141,21 @@ class QueryExecutionErrorsSuite
(df1, df2)
}
+ test("SPARK-58945: invalid file extension reports invalidValue") {
Review Comment:
`getAesInputs()` ends at line 142 and line 159 is the first AES test that
uses it, so the new test lands between a helper and its users and splits that
section. The repo's `AGENTS.md` is explicit about this: put a new member with
the code it belongs with.
Moving it after the AES tests is enough. If it moves out of this file per
the other comment, this resolves itself.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -3215,8 +3215,7 @@ private[sql] object QueryExecutionErrors extends
QueryErrorsBase with ExecutionE
messageParameters = Map(
"functionName" -> toSQLId(functionName),
"parameter" -> toSQLId("extension"),
- "fileExtension" -> toSQLId(extension),
- "acceptable" -> "Extension is limited to exactly 3 letters (e.g. csv,
tsv, etc...)"))
+ "invalidValue" -> toSQLId(extension)))
Review Comment:
`CURSOR_OUTSIDE_SCRIPT` declares no placeholders, but both
`ResolveCursors.scala:85` and `CursorCommandUtils.scala:41` pass a `cursorName`
parameter. That is the same mismatch this PR fixes, on the too-many-parameters
side, which is exactly why the EXTENSION site drops
`fileExtension`/`acceptable`. Production does not compare counts, so nothing
shows today; the moment someone writes a test for this condition,
`ErrorClassesJSONReader`'s `isTesting` branch throws `INTERNAL_ERROR` at
construction instead. The condition has no test coverage at all right now,
which is why CI is green.
The two sites also format the value differently: `nameParts.mkString(".")`
in one, `toSQLId(cursorName)` in the other. The cheapest fix is `Map.empty` in
both. If the cursor name is worth showing, add a `<cursorName>` placeholder to
the template and make both sites use `toSQLId`.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala:
##########
@@ -228,7 +228,12 @@ private[sql] case class H2Dialect() extends JdbcDialect
with NoLegacyJDBCError {
val relationName = messageParameters.getOrElse("tableName", "")
throw new NoSuchTableException(
errorClass = "TABLE_OR_VIEW_NOT_FOUND",
- messageParameters = Map("relationName" -> relationName),
+ messageParameters = Map(
+ "relationName" -> relationName,
+ // classifyException receives only pre-rendered strings, so no
Spark-side
+ // search path exists here; "not available" matches the
rendering used for
+ // an empty search path.
+ "searchPath" -> "not available"),
Review Comment:
The comment's reasoning does not hold: `relationName` is
`messageParameters("tableName")`, which `JDBCTableCatalog` fills with the
schema-qualified name. The schema is available, so `"not available"` is a
choice, not a constraint.
Rewriting the comment to what is verifiable would fix that:
`classifyException` receives pre-rendered strings, so no resolution search path
is threaded through this API. Better still, do not add a third copy of the
literal. What blocks reuse is the enclosing `private[analysis] object
NoSuchItemExceptionHelper` at `noSuchItemsExceptions.scala:26`, not
`formatSearchPath` itself; widening that object to `private[sql]` and calling
it with an empty `Seq` ties the two renderings together.
One more thing: `loadTable`'s 42102 is intercepted by `JDBCRDD.resolveTable`
first, so the `FAILED_JDBC.LOAD_TABLE` plus 42102 pair the new test builds
cannot occur in production.
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/ErrorMessageParametersSuite.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.errors
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.sql.catalyst.catalog.InvalidUDFClassException
+import
org.apache.spark.sql.execution.streaming.state.StateStoreColumnFamilyMismatch
+
+class ErrorMessageParametersSuite extends SparkFunSuite {
Review Comment:
The three tests cover three unrelated pieces of code and share only the
mechanism, so the coverage sits far from the code it guards. The writer commit
message one fits `QueryExecutionErrorsSuite`, which this PR already edits; the
UDF class one fits `QueryCompilationErrorsSuite`; the state store one belongs
in a new `StateStoreErrorsSuite` under `execution/streaming/state/`, where the
next person changing `StateStoreErrors` would actually grep for
`colFamilyName`. The first two carry `SharedSparkSession`, which costs more
than `SparkFunSuite`, and that is the trade.
For the state store case, constructing through the factory
`StateStoreErrors.stateStoreColumnFamilyMismatch` would also be stronger.
Calling the constructor directly with three positional strings means
transposing the old and new schema arguments inside the factory would still
render fine and still pass.
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/ErrorMessageParametersSuite.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.errors
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.sql.catalyst.catalog.InvalidUDFClassException
+import
org.apache.spark.sql.execution.streaming.state.StateStoreColumnFamilyMismatch
+
+class ErrorMessageParametersSuite extends SparkFunSuite {
+
+ test("SPARK-58945: invalid writer commit message reports detail") {
+ checkError(
+ exception = QueryExecutionErrors.invalidWriterCommitMessageError("zero")
+ .asInstanceOf[SparkRuntimeException],
+ condition = "INVALID_WRITER_COMMIT_MESSAGE",
+ parameters = Map("detail" -> "zero"))
+ }
+
+ test("SPARK-58945: invalid UDF class error reports clazz") {
+ checkError(
+ exception =
QueryCompilationErrors.invalidUDFClassError("example.InvalidFunction")
+ .asInstanceOf[InvalidUDFClassException],
+ condition = "_LEGACY_ERROR_TEMP_2450",
+ parameters = Map("clazz" -> "example.InvalidFunction"))
+ }
+
+ test("SPARK-58945: state store mismatch reports schema details") {
+ checkError(
+ exception = new StateStoreColumnFamilyMismatch(
Review Comment:
`StateStoreErrors.stateStoreColumnFamilyMismatch` has no callers, so this
path is unreachable in production, as the description acknowledges. Adding the
unit test is fine in itself, but once it lands the code looks like something
that is in use.
The class and the factory came in together with SPARK-48726 and have had no
callers since, so no external code can be catching this condition by name
either. Either say in the description when the transformWithState side is
expected to wire it up, or open a separate ticket to drop the factory, the
exception class and the condition together.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
