Skip to content

[SPARK-58403][SQL] Assign appropriate error condition for _LEGACY_ERROR_TEMP_3201-3205: MALFORMED_EXPRESSION_INFO - #57604

Closed
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:assign-name-legacy-3201-3205
Closed

[SPARK-58403][SQL] Assign appropriate error condition for _LEGACY_ERROR_TEMP_3201-3205: MALFORMED_EXPRESSION_INFO#57604
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:assign-name-legacy-3201-3205

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR proposes to assign a proper error condition for the legacy error conditions _LEGACY_ERROR_TEMP_3201, _3202, _3203, _3204 and _3205, which are all thrown from the constructor of ExpressionInfo when the metadata describing an expression is malformed.

The five legacy conditions are folded into a single umbrella condition MALFORMED_EXPRESSION_INFO with five subclasses, one per validated field:

Legacy New condition Field
_LEGACY_ERROR_TEMP_3201 MALFORMED_EXPRESSION_INFO.NOTE note
_LEGACY_ERROR_TEMP_3202 MALFORMED_EXPRESSION_INFO.GROUP group
_LEGACY_ERROR_TEMP_3203 MALFORMED_EXPRESSION_INFO.SOURCE source
_LEGACY_ERROR_TEMP_3204 MALFORMED_EXPRESSION_INFO.SINCE since
_LEGACY_ERROR_TEMP_3205 MALFORMED_EXPRESSION_INFO.DEPRECATED deprecated

The shared umbrella message names the offending field and expression ('<fieldName>' is malformed in the expression [<exprName>]:), and each subclass carries the field-specific detail. fieldName is a new message parameter, so getMessageParameters() for these errors now carries one extra key.

The assigned SQLSTATE is 22023 (invalid parameter value), consistent with the sibling MALFORMED_* conditions that validate a value against an allowed set or format (MALFORMED_RECORD_IN_PARSING, MALFORMED_VARIANT, and the INVALID_PARAMETER_VALUE archetype all use 22023).

On reachability, and why these get a proper name rather than INTERNAL_ERROR: none of the five throw sites is reachable from a user query. FunctionRegistryBase.expressionInfo reads the compile-time @ExpressionDescription annotation, and SessionCatalog.makeExprInfoForHiveFunction / SQLFunction.toExpressionInfo pass constants ("" / "hive" / "sql_udf"). A malformed value can only come from extension or third-party code: constructing new ExpressionInfo(...) directly (which is what SparkSessionExtensions.injectFunction takes, see the example in SparkSessionExtensionsProvider), or calling FunctionRegistryBase.createOrReplaceTempFunction(name, builder, source) with an arbitrary source. That makes these developer-facing rather than engine-internal invariants: the person who triggers the error is the one who can fix it, so a named, actionable condition fits better than an internal error. This mirrors existing extension/configuration-author-facing conditions such as CANNOT_LOAD_CATALOG and CANNOT_LOAD_FUNCTION_CLASS, which also carry standard SQLSTATEs.

Why are the changes needed?

_LEGACY_ERROR_TEMP_* conditions are placeholders that should be replaced with proper, named error conditions per the guideline in common/utils/src/main/resources/error/README.md. This is part of the ongoing effort to migrate legacy error conditions to the structured error framework.

Does this PR introduce any user-facing change?

No. As described above, these conditions are only reachable by extension or third-party code that builds an ExpressionInfo itself, and the _LEGACY_ERROR_TEMP_* names were never part of the public API.

For completeness, the message text is preserved with four deliberate changes:

  • the GROUP value is now bracketed (however, got <group>. -> however, got [<group>].) for consistency with the other four subclasses;
  • the split between the field and the detail moved from . to :, and the detail therefore starts with a lowercase it should instead of It should;
  • the rendered message now carries the [MALFORMED_EXPRESSION_INFO.<SUBCLASS>] prefix, which SparkThrowableHelper.formatErrorMessage omits only for _LEGACY_ERROR_-prefixed names;
  • the rendered message now carries a SQLSTATE: 22023 suffix, since these five legacy entries previously had no sqlState at all.

Before and after, for GROUP:

OLD: 'group' is malformed in the expression [testName]. It should be a value in [...]; however, got invalid_group_funcs.
NEW: [MALFORMED_EXPRESSION_INFO.GROUP] 'group' is malformed in the expression [testName]: it should be a value in [...]; however, got [invalid_group_funcs]. SQLSTATE: 22023

How was this patch tested?

Updated the existing assertions in ExpressionInfoSuite to check the new conditions and parameters, and added sqlState = Some("22023") to each so the assigned SQLSTATE is pinned by a test. Ran:

  • ExpressionInfoSuite - 10/10 passed
  • SparkThrowableSuite - 34/34 passed (JSON validity, alphabetical ordering, mandatory SQLSTATE, round-trip)

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

@LuciferYang
LuciferYang marked this pull request as draft July 28, 2026 12:08
…ROR_TEMP_3201-3205`: `MALFORMED_EXPRESSION_INFO`
@LuciferYang
LuciferYang force-pushed the assign-name-legacy-3201-3205 branch from d010223 to ed203df Compare July 29, 2026 14:43
@LuciferYang
LuciferYang marked this pull request as ready for review July 29, 2026 14:45

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

LuciferYang added a commit that referenced this pull request Jul 30, 2026
…ROR_TEMP_3201-3205`: `MALFORMED_EXPRESSION_INFO`

### What changes were proposed in this pull request?

This PR proposes to assign a proper error condition for the legacy error conditions `_LEGACY_ERROR_TEMP_3201`, `_3202`, `_3203`, `_3204` and `_3205`, which are all thrown from the constructor of `ExpressionInfo` when the metadata describing an expression is malformed.

The five legacy conditions are folded into a single umbrella condition `MALFORMED_EXPRESSION_INFO` with five subclasses, one per validated field:

| Legacy | New condition | Field |
|---|---|---|
| `_LEGACY_ERROR_TEMP_3201` | `MALFORMED_EXPRESSION_INFO.NOTE` | `note` |
| `_LEGACY_ERROR_TEMP_3202` | `MALFORMED_EXPRESSION_INFO.GROUP` | `group` |
| `_LEGACY_ERROR_TEMP_3203` | `MALFORMED_EXPRESSION_INFO.SOURCE` | `source` |
| `_LEGACY_ERROR_TEMP_3204` | `MALFORMED_EXPRESSION_INFO.SINCE` | `since` |
| `_LEGACY_ERROR_TEMP_3205` | `MALFORMED_EXPRESSION_INFO.DEPRECATED` | `deprecated` |

The shared umbrella message names the offending field and expression (`'<fieldName>' is malformed in the expression [<exprName>]:`), and each subclass carries the field-specific detail. `fieldName` is a new message parameter, so `getMessageParameters()` for these errors now carries one extra key.

The assigned SQLSTATE is `22023` (invalid parameter value), consistent with the sibling `MALFORMED_*` conditions that validate a value against an allowed set or format (`MALFORMED_RECORD_IN_PARSING`, `MALFORMED_VARIANT`, and the `INVALID_PARAMETER_VALUE` archetype all use `22023`).

**On reachability, and why these get a proper name rather than `INTERNAL_ERROR`:** none of the five throw sites is reachable from a user query. `FunctionRegistryBase.expressionInfo` reads the compile-time `ExpressionDescription` annotation, and `SessionCatalog.makeExprInfoForHiveFunction` / `SQLFunction.toExpressionInfo` pass constants (`""` / `"hive"` / `"sql_udf"`). A malformed value can only come from extension or third-party code: constructing `new ExpressionInfo(...)` directly (which is what `SparkSessionExtensions.injectFunction` takes, see the example in `SparkSessionExtensionsProvider`), or calling `FunctionRegistryBase.createOrReplaceTempFunction(name, builder, source)` with an arbitrary `source`. That makes these developer-facing rather than engine-internal invariants: the person who triggers the error is the one who can fix it, so a named, actionable condition fits better than an internal error. This mirrors existing extension/configuration-author-facing conditions such as `CANNOT_LOAD_CATALOG` and `CANNOT_LOAD_FUNCTION_CLASS`, which also carry standard SQLSTATEs.

### Why are the changes needed?

`_LEGACY_ERROR_TEMP_*` conditions are placeholders that should be replaced with proper, named error conditions per the guideline in `common/utils/src/main/resources/error/README.md`. This is part of the ongoing effort to migrate legacy error conditions to the structured error framework.

### Does this PR introduce _any_ user-facing change?

No. As described above, these conditions are only reachable by extension or third-party code that builds an `ExpressionInfo` itself, and the `_LEGACY_ERROR_TEMP_*` names were never part of the public API.

For completeness, the message text is preserved with four deliberate changes:

- the `GROUP` value is now bracketed (`however, got <group>.` -> `however, got [<group>].`) for consistency with the other four subclasses;
- the split between the field and the detail moved from `.` to `:`, and the detail therefore starts with a lowercase `it should` instead of `It should`;
- the rendered message now carries the `[MALFORMED_EXPRESSION_INFO.<SUBCLASS>] ` prefix, which `SparkThrowableHelper.formatErrorMessage` omits only for `_LEGACY_ERROR_`-prefixed names;
- the rendered message now carries a ` SQLSTATE: 22023` suffix, since these five legacy entries previously had no `sqlState` at all.

Before and after, for `GROUP`:

```
OLD: 'group' is malformed in the expression [testName]. It should be a value in [...]; however, got invalid_group_funcs.
NEW: [MALFORMED_EXPRESSION_INFO.GROUP] 'group' is malformed in the expression [testName]: it should be a value in [...]; however, got [invalid_group_funcs]. SQLSTATE: 22023
```

### How was this patch tested?

Updated the existing assertions in `ExpressionInfoSuite` to check the new conditions and parameters, and added `sqlState = Some("22023")` to each so the assigned SQLSTATE is pinned by a test. Ran:

- `ExpressionInfoSuite` - 10/10 passed
- `SparkThrowableSuite` - 34/34 passed (JSON validity, alphabetical ordering, mandatory SQLSTATE, round-trip)

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

Closes #57604 from LuciferYang/assign-name-legacy-3201-3205.

Authored-by: YangJie <yangjie01@baidu.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
(cherry picked from commit 91b1abd)
Signed-off-by: yangjie01 <yangjie01@baidu.com>
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Merge Summary:

Posted by merge_spark_pr.py

@LuciferYang

Copy link
Copy Markdown
Contributor Author

Thank you @dongjoon-hyun @uros-b

@LuciferYang
LuciferYang deleted the assign-name-legacy-3201-3205 branch July 30, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants