Skip to content

Conversation

@mihailom-db
Copy link
Contributor

@mihailom-db mihailom-db commented Oct 3, 2024

What changes were proposed in this pull request?

Introducing two new error classes instead of _LEGACY_ERROR_TEMP_2000.
Classes introduced:

  • DATETIME_FIELD_OUT_OF_BOUNDS
  • INVALID_INTERVAL_WITH_MICROSECONDS_ADDITION

Why are the changes needed?

We want to assign names for all existing error classes.

Does this PR introduce any user-facing change?

Yes, error message changed.

How was this patch tested?

Existing tests cover error raising.

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

No.

@github-actions github-actions bot added the SQL label Oct 3, 2024
@mihailom-db mihailom-db requested a review from MaxGekk October 3, 2024 09:27
}

def ansiDateTimeError(e: Exception): SparkDateTimeException = {
def ansiDateTimeArgumentOutOfRange(e: Exception): SparkDateTimeException = {
Copy link
Member

Choose a reason for hiding this comment

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

Changes in this PR clash with #48242 by @itholic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's resolve that ticket first, but keep this one open for now, as I am not sure what complete scope will be of the other ticket and this PRs goal is to remove _LEGACY_ERROR_TEMP_2000 completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxGekk I aligned my changes with the @itholic PR, so we can go on and merge this change, then the other PR can improve the message, while this one stays scoped to assigning proper error classes.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

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.

Map("message" ->
"Cannot add hours, minutes or seconds, milliseconds, microseconds to a date",
"INVALID_PARAMETER_VALUE.INTERVAL_WITH_MICROSECONDS",
Map("parameter" -> "`interval`", "functionName" -> "`dateaddinterval`",
Copy link
Member

Choose a reason for hiding this comment

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

dateaddinterval there is no such function. That might confuse users. We could pass + instead, or create a error condition especially for the DateAddInterval expression (not sub-class of INVALID_PARAMETER_VALUE ).

@mihailom-db
Copy link
Contributor Author

@MaxGekk this PR is ready for merge, let me know when you re-review it so I can rerun the CIs, so we do not break something, as these runs are from yesterday.


def ansiIllegalArgumentError(message: String): SparkIllegalArgumentException = {
def invalidIntervalWithMicrosecondsAdditionError(
funcName: String): SparkIllegalArgumentException = {
Copy link
Member

Choose a reason for hiding this comment

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

funcName is not used anymore. Could you remove it, please.

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 13, 2024

+1, LGTM. Merging to master.
Thank you, @mihailom-db.

@MaxGekk MaxGekk closed this in 05508cf Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants