LuciferYang commented on code in PR #58225:
URL: https://github.com/apache/spark/pull/58225#discussion_r3946699987
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCursors.scala:
##########
@@ -82,7 +83,7 @@ class ResolveCursors extends Rule[LogicalPlan] {
// Cursors are only allowed within SQL scripts
throw new AnalysisException(
errorClass = "CURSOR_OUTSIDE_SCRIPT",
- messageParameters = Map("cursorName" -> nameParts.mkString(".")))
+ messageParameters = Map("cursorName" -> toSQLId(nameParts)))
Review Comment:
`ResolveCursors.scala:86`'s `cursorName` became `toSQLId(nameParts)`, while
`:107` in the same method still passes `nameParts.mkString(".")` for
`CURSOR_NOT_FOUND`. The other three `CURSOR_NOT_FOUND` sites
(`FetchCursorExec:59`, `OpenCursorExec:68`, `CloseCursorExec:47`) all use
`toSQLId`, leaving `:107` as the only unquoted one in the cursor code.
So one condition now renders two ways: a bare name from analysis, a quoted
one from execution. `SqlScriptingCursorE2eSuite:404` still asserts a bare `cur`
for `CURSOR_NOT_FOUND`. The inconsistency is pre-existing, but this PR already
mixes `DataTypeErrorsBase` into the rule, so `:107` is a one-line
follow-through plus one expectation at `:404`.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala:
##########
@@ -225,10 +231,14 @@ private[sql] case class H2Dialect() extends JdbcDialect
with NoLegacyJDBCError {
cause = Some(e))
// TABLE_OR_VIEW_NOT_FOUND_1
case 42102 =>
Review Comment:
`isObjectNotFoundException` treats `{42102, 42103, 42104, 90079}` as
object-not-found, while of those four only 42102 and 90079 get a `case` in
`classifyException`. 42103 is "not found, but here are the near-miss
candidates" and 42104 is "no tables in the database"; neither lands in the
branch you just changed.
Spark quotes identifiers, so H2 stores the name exactly as created and a
user writing `h2.test.PEOPLE` may get 42103. `renameTable` then raises
`FAILED_JDBC.RENAME_TABLE` where its interface documents
`NoSuchTableException`; drop and `tableExists` read the set above and are
unaffected. This is pre-existing code, and folding both codes into the same
case is a one-line change; a separate ticket works too if it widens this PR.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CursorCommandUtilsSuite.scala:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.execution.command.v2
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.SqlScriptingContextManager
+
+class CursorCommandUtilsSuite extends SparkFunSuite {
+
+ test("SPARK-58945: CursorCommandUtils reports cursor name outside scripts") {
+ assert(SqlScriptingContextManager.get().isEmpty)
+ checkError(
+ exception = intercept[AnalysisException] {
+ CursorCommandUtils.getScriptingContext("cur")
Review Comment:
`OPEN`/`FETCH`/`CLOSE` are rejected by `ResolveCursors` during analysis, so
the throw site in `CursorCommandUtils` has exactly one path into it from SQL: a
top-level `DECLARE ... CURSOR`. `DeclareCursor` is a `LeafCommand` whose fields
are all String/Boolean, so the rule never fires and execution reaches
`DeclareCursorExec.scala:45`. The new `SqlScriptingCursorE2eSuite:120` OPEN
test covers the `ResolveCursors` site, not this one.
That path has no test today. Calling `getScriptingContext` on the
`private[v2]` object directly skips the parser, the cursor conf gate and the
`V2CommandStrategy` mapping, so breaking any of the three leaves this test
green. Add a second case, `sql("DECLARE cur CURSOR FOR SELECT 1")`, to
`SqlScriptingCursorE2eSuite` after `Test 92` rather than inside that numbered
run; its `beforeAll` already enables the cursor conf, and
`CursorCommandUtilsSuite` can then go away.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala:
##########
@@ -225,10 +231,14 @@ private[sql] case class H2Dialect() extends JdbcDialect
with NoLegacyJDBCError {
cause = Some(e))
// TABLE_OR_VIEW_NOT_FOUND_1
case 42102 =>
- val relationName = messageParameters.getOrElse("tableName", "")
+ val relationName = messageParameters
+ .getOrElse("tableName", messageParameters.getOrElse("oldName",
""))
throw new NoSuchTableException(
errorClass = "TABLE_OR_VIEW_NOT_FOUND",
- messageParameters = Map("relationName" -> relationName),
+ messageParameters = Map(
+ "relationName" -> relationName,
+ // Use the shared empty-search-path rendering for this
dialect-classification path.
+ "searchPath" ->
NoSuchItemExceptionHelper.formatSearchPath(Seq.empty)),
Review Comment:
The comment inside the Map says the same thing as the
`formatSearchPath(Seq.empty)` call below it. The part worth recording is the
other one: `classifyException` receives pre-rendered strings, so no resolution
search path is threaded through this API, and `not available` is the only thing
this path can supply.
Saying that instead spares the next reader of `Search path: not available.`
from wondering whether a parameter was dropped.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -11818,7 +11818,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:
Dropping the template 3070 had copied from 3069 is right, and 3069 keeps the
wording. What is left is the condition itself: `error/README.md` says `You
should not introduce new uncategorized errors. Instead, convert them to proper
errors whenever encountering them in new code.`, and `_LEGACY_ERROR_TEMP_3070`
has no sqlState.
Reaching this throw site requires a third-party FileFormat that declares a
metadata field without marking it constant or generated, since every built-in
format marks them. That makes it an implementation error rather than a user
error, so converting it to `INTERNAL_ERROR` fits better than minting a
user-visible condition. Converting means touching the `error-conditions.json`
entry and the new `FileSourceCustomMetadataStructSuite` case as well; if that
widens this PR too far, naming the follow-up JIRA in the description would do,
and the other comment about the `<field>` rendering then follows the same route.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala:
##########
@@ -291,6 +291,14 @@ class ExpressionParserSuite extends AnalysisTest {
assertEqual("a is not distinct from b", $"a" <=> $"b")
}
+ test("invalid semi-structured extract path") {
Review Comment:
The `ExpressionParserSuite` case sits between `is distinct expressions` and
`binary arithmetic expressions`, splitting two unrelated groups; it tests colon
extraction, whose neighbours are `dereference` (`:633`) and `subscript`
(`:656`), so after `subscript` is where it belongs.
Same for `SqlScriptingCursorE2eSuite`: the file is a case-by-case
transcription of cursors.sql with tests named `Test 1` through `Test 92`, and
the new case lands between `Test 4` and `Test 5`, cutting that run in half.
After `Test 92` works. This is the rule CLAUDE.md states, the same one that
moved the AES case out of its section last round.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileSourceCustomMetadataStructSuite.scala:
##########
@@ -413,6 +413,21 @@ class FileSourceCustomMetadataStructSuite extends
SharedSparkSession {
Row(1, 112L, 1L, f1.getLen, f1.getPath.getName)))
}
}
+
+ test("SPARK-58945: invalid file metadata fields report the field") {
+ withTempData("parquet", FILE_SCHEMA) { (_, f0, f1) =>
+ val invalidField = StructField("bad", StringType)
+ val format = new TestFileFormat(Seq(invalidField))
+ val df = createDF(format, Seq(FileStatusWithMetadata(f0),
FileStatusWithMetadata(f1)))
+
+ checkError(
+ exception = intercept[AnalysisException] {
+ df.select("_metadata.bad").collect()
+ },
+ condition = "_LEGACY_ERROR_TEMP_3070",
+ parameters = Map("field" -> invalidField.toString))
Review Comment:
`<field>` renders as `StructField(bad,StringType,true)`, because
`FileSourceStrategy.scala:292` passes `field.toString`. 3.5 printed the same
thing and 4.0 onwards reports `INTERNAL_ERROR`, so this PR restores the 3.5
rendering, and the new test freezes the dump as its expectation.
`FileSourceStrategy` is an `object` with no access to `toSQLId`, which is a
`DataTypeErrorsBase` method, so this needs either that trait mixed in the way
this PR does for `ResolveCursors`, or an added import plus
`QuotingUtils.quoteIdentifier(field.name)`; that one also avoids
`toSQLId(String)` splitting on `.`. The expectation at
`FileSourceCustomMetadataStructSuite:428` changes to `` `bad` `` alongside. If
it becomes an `INTERNAL_ERROR` per the other comment, this one goes away.
--
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]
