Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Oct 18, 2022

What changes were proposed in this pull request?

This pr replace TypeCheckFailure by DataTypeMismatch in type checks in the string expressions, includes:

  • regexpExpressions.scala (RegExpReplace)
  • stringExpressions.scala (Etl)

Why are the changes needed?

Migration onto error classes unifies Spark SQL error messages.

Does this PR introduce any user-facing change?

Yes. The PR changes user-facing error messages.

How was this patch tested?

  • Add new UT
  • Update existed UT
  • Pass GA.

@panbingkun
Copy link
Contributor Author

cc @MaxGekk

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@MaxGekk
Copy link
Member

MaxGekk commented Oct 18, 2022

@panbingkun Could you resolve conflicts, please.

@panbingkun
Copy link
Contributor Author

@panbingkun Could you resolve conflicts, please.
Done

"The <exprName> must not be null"
]
},
"UNEXPECTED_NUM_PARAMS" : {
Copy link
Member

Choose a reason for hiding this comment

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

It seems it is similar to WRONG_NUM_PARAMS. Could you re-use the existing one or improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make WRONG_NUM_PARAMS more general

Copy link
Contributor Author

@panbingkun panbingkun Oct 19, 2022

Choose a reason for hiding this comment

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

Can I change it to:
"WRONG_NUM_PARAMS" : { "message" : [ "The <functionName> requires <expectedNum> arguments (actual number = <actualNum>)" ] }

Copy link
Member

Choose a reason for hiding this comment

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

Please, open a separate PR and change the error template. I would change it to:

      "WRONG_NUM_PARAMS" : {
        "message" : [
          "The <functionName> requires <expectedNum> parameters but the actual number is <actualNum>."
        ]
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

"cannot find a static method <methodName> that matches the argument types in <className>"
]
},
"UNEXPECTED_VALUE" : {
Copy link
Member

Choose a reason for hiding this comment

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

Can't you re-use VALUE_OUT_OF_RANGE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@panbingkun
Copy link
Contributor Author

After #38319, I will continue do it.

@MaxGekk
Copy link
Member

MaxGekk commented Oct 23, 2022

+1, LGTM. Merging to master.
Thank you, @panbingkun and @LuciferYang for review.

@MaxGekk MaxGekk closed this in f81c265 Oct 23, 2022
@panbingkun panbingkun deleted the SPARK-40756 branch November 7, 2022 02:00
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
…onto error classes

### What changes were proposed in this pull request?
This pr replace TypeCheckFailure by DataTypeMismatch in type checks in the string expressions, includes:
- regexpExpressions.scala (RegExpReplace)
- stringExpressions.scala (Etl)

### Why are the changes needed?
Migration onto error classes unifies Spark SQL error messages.

### Does this PR introduce _any_ user-facing change?
Yes. The PR changes user-facing error messages.

### How was this patch tested?
- Add new UT
- Update existed UT
- Pass GA.

Closes apache#38299 from panbingkun/SPARK-40756.

Authored-by: panbingkun <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants