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

refactor types package to decouple it from stmtctx #47355

Closed
14 of 15 tasks
lcwangchao opened this issue Sep 28, 2023 · 3 comments
Closed
14 of 15 tasks

refactor types package to decouple it from stmtctx #47355

lcwangchao opened this issue Sep 28, 2023 · 3 comments
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@lcwangchao
Copy link
Collaborator

lcwangchao commented Sep 28, 2023

Enhancement

Currently, the package types methods rely on the package stmtctx as the context for value conversion. We'd better to
avoid it and decouple these two packages for some reasons:

  • types is a basic package, it should not have an assumption that all the value operations happen in a statement of SQL.
  • stmtctx.StatementContext has too many fields and is challenging to use. We should a simple object for context with clear semantics

For above reasons, we should redesign the context in package to introduce a new context named types.Context . For example:

type Context struct {
    flags Flags
    loc *time.Location
    warningFunc func(err)
}

The new context definition has some fields:

  • flags: some flags to indicate how to handle the value conversion.
  • loc: the timezone info, it will be used by some time-related functions.
  • warningFunc: in some cases, we only want to append a warning to somewhere instead of returning an error. This function will be called when an error happens and the context indicates to enable warnings.

It's better to make the Context as immutable to make it thread-safe and easier to use. So the inner fields are designed to be private.

The Flags type will be defined as an int with each bit stands for a flag:

const (
	FlagIgnoreTruncateErr Flags = 1 << iota
	FlagTruncateAsWarning
	FlagClipNegativeToZero
	FlagIgnoreOverflowError
	FlagOverflowAsWarning
	FlagIgnoreZeroDateErr
	FlagZeroDateAsWarning
	FlagIgnoreZeroInDateErr
	FlagZeroInDateAsWarning
	FlagIgnoreInvalidDateErr
	FlagInvalidDateAsWarning
	FlagSkipASCIICheck
	FlagSkipUTF8Check
	FlagSkipUTF8MB4Check
)

sub tasks

@YangKeao
Copy link
Member

Maybe it's time to remove all (or as most as possible) stmtctx from types. Let's do it! #48080

@bb7133
Copy link
Member

bb7133 commented Dec 13, 2023

Can we close this issue now?

@lcwangchao
Copy link
Collaborator Author

Can we close this issue now?

#49122 is still in review. Maybe we can close it after it finished.

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

No branches or pull requests

3 participants