Skip to content

[SPARK-37979][SQL] Switch to more generic error classes in AES functions#35272

Closed
MaxGekk wants to merge 7 commits intoapache:masterfrom
MaxGekk:invalid-input-func
Closed

[SPARK-37979][SQL] Switch to more generic error classes in AES functions#35272
MaxGekk wants to merge 7 commits intoapache:masterfrom
MaxGekk:invalid-input-func

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jan 21, 2022

What changes were proposed in this pull request?

In the PR, I propose to switch from specific error classes in the AES functions: aes_encrypt()/aes_decrypt() to more generic:

  • AES_CRYPTO_ERROR -> INVALID_PARAMETER_VALUE
  • INVALID_AES_KEY_LENGTH -> INVALID_PARAMETER_VALUE
  • UNSUPPORTED_AES_MODE -> UNSUPPORTED_FEATURE

The new error classes INVALID_PARAMETER_VALUE and UNSUPPORTED_MODE are made to be re-used from other functions but not only in the AES functions.

Why are the changes needed?

  1. To prevent unlimited inflation of the set of error classes and as a consequence of that inflation of error-classes.json.
  2. To establish rules for other sub-tasks of SPARK-37935

Does this PR introduce any user-facing change?

Yes, but the AES functions aes_encrypt()/aes_decrypt() haven't released yet.

How was this patch tested?

By running the affected test suites:

$ build/sbt "test:testOnly *SparkThrowableSuite"
$ build/sbt "test:testOnly *QueryExecutionErrorsSuite"

@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 21, 2022

@cloud-fan @sarutak @srielau @entong Could you review this PR, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

mode is also a parameter of the aes function, shall we reuse INVALID_PARAMETER_VALUE

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, any failure/error can be considered as a result of an invalid parameter. We could consider the entire universe as the input to Spark's code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cloud-fan What is the use case for the sqlState = 0A000 (feature not supported, see https://github.com/apache/spark/tree/master/core/src/main/resources/error) from your point of view?

Copy link
Contributor

Choose a reason for hiding this comment

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

how about The value of parameter '%s' in %s is invalid: %s. Then we can have something like
The value of parameter 'key' in function 'aes_encrypt/aes_decrypt' is invalid: expects a binary value with 16, 24 or 32 bytes, but got 9 bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"aes_encrypt/aes_decrypt",
"function aes_encrypt/aes_decrypt",

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
messageParameters = Array(s"AES-$mode with the padding $padding"))
messageParameters = Array(s"AES-$mode with the padding $padding in function aes_encrypt/aes_decrypt"))

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"aes_encrypt/aes_decrypt",
"function aes_encrypt/aes_decrypt",

@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 24, 2022

Merging to master. Thank you, @cloud-fan .

@MaxGekk MaxGekk closed this in 8fef5bb Jan 24, 2022
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.

2 participants

Comments