Skip to content

Conversation

@panbingkun
Copy link
Contributor

What changes were proposed in this pull request?

The pr aims to migrate the map options errors onto error classes.

Why are the changes needed?

The changes improve the error framework.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

@MaxGekk
Copy link
Member

MaxGekk commented Nov 23, 2022

@panbingkun Could you resolve conflicts, please.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@panbingkun
Copy link
Contributor Author

@panbingkun Could you resolve conflicts, please.

Done.

"subClass" : {
"NON_STRING_TYPE" : {
"message" : [
"A type of keys and values in map() must be string, but got <map>."
Copy link
Member

Choose a reason for hiding this comment

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

nit: map -> mapType

"A type of keys and values in map() must be string, but got <map>."
]
},
"NOT_MAP_FUNCTION" : {
Copy link
Member

Choose a reason for hiding this comment

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

Why not NON_MAP_FUNCTION?

messageParameters = Map("map" -> toSQLType(m.dataType)))
}

def nonMapFunctionNotAllowedError(): Throwable = {
Copy link
Member

Choose a reason for hiding this comment

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

Be sure the names are consistent. not or non

},
"NOT_MAP_FUNCTION" : {
"message" : [
"Must use a map() function for options."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Must use a map() function for options."
"Must use the `map()` function for options."

@panbingkun panbingkun requested a review from MaxGekk November 25, 2022 02:35
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

Waiting for CI.

@MaxGekk
Copy link
Member

MaxGekk commented Nov 25, 2022

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in 0ae82d9 Nov 25, 2022
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 15, 2022
### What changes were proposed in this pull request?
The pr aims to migrate the map options errors onto error classes.

### Why are the changes needed?
The changes improve the error framework.

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

### How was this patch tested?
Pass GA.

Closes apache#38730 from panbingkun/SPARK-41181.

Authored-by: panbingkun <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 18, 2022
### What changes were proposed in this pull request?
The pr aims to migrate the map options errors onto error classes.

### Why are the changes needed?
The changes improve the error framework.

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

### How was this patch tested?
Pass GA.

Closes apache#38730 from panbingkun/SPARK-41181.

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.

3 participants