nchammas commented on PR #58282: URL: https://github.com/apache/spark/pull/58282#issuecomment-5482383412
I think the goal of having as much error text live in JSON as possible is a good goal, and facilitates as you say both error audits/updates as well as localization. There is even a line in the error README about [not passing messages as parameters to other messages](https://github.com/apache/spark/blob/9911af1935efdaac1fcbcde760719f75228f7416/common/utils/src/main/resources/error/README.md?plain=1#L142). Making sub-conditions optional and using that mechanism for optional hints doesn't help us if [a sub-condition itself has an optional suggestion][1]; our error design only allows up to one level of nesting. So while `CONDITION.WITH_HINT` works, `CONDITION.SUB.WITH_HINT` would not be allowed. [1]: https://github.com/apache/spark/blob/9911af1935efdaac1fcbcde760719f75228f7416/common/utils/src/main/resources/error/error-conditions.json#L2144-L2148 We already have a convention that moves optional error message parts out of code and into JSON: `WITH_SUGGESTION` and `WITHOUT_SUGGESTION`. It works, but it's a bit clunky and we don't follow it consistently. @HyukjinKwon @MaxGekk @srielau - I think we should formalize how we want optional error condition message parts to be handled. My proposal is this: 1. Optional hints should be siblings of the main condition or sub-condition they are about. i.e. `CONDITION` and `CONDITION_WITH_HINT`, or `CONDITION.SUB` and `CONDITION.SUB_WITH_HINT`. 2. Don't include `_WITHOUT_HINT` (or similar) in the name of any error condition. 3. Don't pass in empty strings to optional hint parameters. Move the hint message into the error condition text in the JSON. <details> <summary>Some examples</summary> ```json // before "PARSE_SYNTAX_ERROR" : { "message" : [ "Syntax error at or near <error><hint>." ] } // after "PARSE_SYNTAX_ERROR" : { "message" : [ "Syntax error at or near <error>." ] }, "PARSE_SYNTAX_ERROR_WITH_EXTRA_INPUT" : { "message" : [ "Syntax error at or near <error>: extra input <extra>." ] }, "PARSE_SYNTAX_ERROR_WITH_MISSING_TOKEN" : { "message" : [ "Syntax error at or near <error>: missing <expected>." ] } ``` ```json // before "UNRESOLVED_COLUMN" : { "message" : [ "A column, variable, or function parameter with name <objectName> cannot be resolved." ], "subClass" : { "WITHOUT_SUGGESTION" : { "message" : [ "" ] }, "WITH_SUGGESTION" : { "message" : [ "Did you mean one of the following? [<proposal>]." ] } }, "sqlState" : "42703" } // after "UNRESOLVED_COLUMN" : { "message" : [ "A column, variable, or function parameter with name <objectName> cannot be resolved." ], "sqlState" : "42703" }, "UNRESOLVED_COLUMN_WITH_SUGGESTION" : { "message" : [ "A column, variable, or function parameter with name <objectName> cannot be resolved. Did you mean one of the following? [<proposal>]." ], "sqlState" : "42703" } ``` ```json // before "DATATYPE_MISMATCH" : { "message" : [ "Cannot resolve <sqlExpr> due to data type mismatch:" ], "subClass" : { "TYPE_CHECK_FAILURE_WITH_HINT" : { "message" : [ "<msg><hint>." ] } } } // after "DATATYPE_MISMATCH" : { "message" : [ "Cannot resolve <sqlExpr> due to data type mismatch:" ], "subClass" : { "TYPE_CHECK_FAILURE" : { "message" : [ "<msg>." ] }, "TYPE_CHECK_FAILURE_WITH_HINT" : { "message" : [ "<msg>. To fix the error, you might need to add explicit type casts. If necessary set <config> to false to bypass this error." ] } } } ``` </details> The main downside is having to repeat part of the message. Otherwise, this is a consistent approach that we can apply to conditions and sub-conditions, and it addresses the audit/translate concern without changing the current error condition design. What do you think? -- 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]
