Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate handling truncate error from typectx to errctx #48919

Open
YangKeao opened this issue Nov 27, 2023 · 2 comments · May be fixed by #48970
Open

migrate handling truncate error from typectx to errctx #48919

YangKeao opened this issue Nov 27, 2023 · 2 comments · May be fixed by #48970
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@YangKeao
Copy link
Member

Enhancement

We have developed a standalone package to handle errors. I'll migrate the error handling logic used inside types from using typectx to return the error directly, and handle the error with errctx outside.

@YangKeao YangKeao added the type/enhancement The issue or PR belongs to an enhancement. label Nov 27, 2023
@YangKeao YangKeao changed the title migrate truncate error handling from typectx to errctx migrate handling truncate error from typectx to errctx Nov 27, 2023
@YangKeao YangKeao linked a pull request Nov 28, 2023 that will close this issue
5 tasks
@YangKeao
Copy link
Member Author

YangKeao commented Dec 1, 2023

To solve this problem in a more accurate way, I'll try to conclude the usage of HandleTruncate, HandleOverflow and the related errors. They are used to handle the errors according to the configuration of sql_mode in a lot of different places. These logic integrates deeply in types package, expression and tables. Now we'd like to decouple the sql_mode (which is part of the statement context) with types package and some other utilities packages. It's time to re-think how to make these functions more intuitive.

HandleTruncate function

Previously, the behavior of HandleTruncate is like the HandleOverflow. It just handles the error according to the TruncateAsWarning (or ignore) fields, without considering the type of the errors. In #40078, some filters are added to the HandleTruncate function, but I don't know why we got this PR (maybe some corner cases). Now I'd like to conclude every possible errors which will go into HandleTruncate, and organize them by the use cases of HandleTruncate:

  1. In types package. Most of the time, it handles ErrTruncatedWrongVal. It'll also handle ErrOverflow in a special corner case: when the Decimal gives a too big (greater than math.MaxInt32/2 but not big enough to overflow) exponent.
  2. In param package, handle the error of parsing time or duration parameters. I'm not sure whether this usage is correct (though, it's written by myself 🤦 ). Maybe I can use similar behavior like the expression package handleInvalidTimeError.
  3. For INSERT statement, handle the error occured when it's evaluating the expression in generated column. (See pkg/execturo/insert_common.go, I'm also not sure whether this usage is expected. Maybe that's why we have *: filter particularly errors when truncateAsWarning is true #40078 🤔 ).
    It also handles the error after casting the value to the corresponding column types. See pkg/table/column.go. However, many TruncatedErr are handled following it.
  4. In expression package. The usage is nearly the same with types package. But I noticed that, some times (e.g. builtinCastIntAsDurationSig) handles both ErrOverflow and ErrTruncatedWrongVal, but others may only handle ErrTruncatedWrongVal, I'm not sure whether it's expected.
    Through handleInvalidTimeError, it also handles a lot of different kinds of InvalidTimeError. This is a big part besides TruncatedWrongVal.
    It also handles ErrTruncated from the ParseEnum. In scalar_function.go.

HandleOverflow function

Most of the cases of HandleOverflow only handle the ErrOverflow. The only corner case is that it handles all error from ConvertJSONToInt, which is also possible to return any kinds of error here. However, it also should only handle ErrOverflow, as shown in #47997 from the original code.

There is also a HandleOverflow behind HandleTruncate, which is meaningless, because when OverflowAsWarning is true, TruncateAsWarning is always true. If OverflowAsWarning is false, HandleOverflow will return the error directly, and it's fine to remove it. If OverflowAsWarning is true, HandleTruncate can eat the error.

Conclusion

We can split the errors in two parts:

  1. ErrTruncatedWrongVal, ErrTruncated, ErrWrongValue, ErrWrongValueForType, ErrInvalidWeekModeFormat, ErrDatetimeFunctionOverflow, ErrIncorrectDatetimeValue can be organized in a single group (actually, the time related errors can be split into another group as the semantics, but I don't think it's a big problem). They should be handled in a similar way.
  2. ErrOverflow, it should be handled in another pattern.

They can actually have different patterns. We can tell that when OverflowAsWarning is true, TruncateAsWarning is always true, but the converse is not correct. There are a lot of cases when the TruncateAsWarning is true, but OverflowAsWarning is false.

To keep the current behavior still correct for INSERT and generated columns, we'll still need a function to handle all kinds of these errors. Maybe HandleTruncateOld (or some better name).

@YangKeao
Copy link
Member Author

YangKeao commented Dec 4, 2023

Here is a note to reproduce these behaviors in MySQL:

  1. To make TruncateAsWarning true and OverflowAsWarning false, use INSERT statement and don't set strict sql_mode.
  2. To make both of them true, use SELECT statement and don't set strict sql_mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant