Support built-in MySQL functions ABS #9774

tianshishuo · 2022-02-24T10:50:55Z

Description

evalengine: Support built-in MySQL functions--abs()

Related Issue(s)

Contributes to evalengine: Support all built-in MySQL functions

Checklist

Should this PR be backported?
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

vmg

Thanks for your first contribution to Vitess! I've left some feedback on things that need to be fixed -- particularly related to testing.

go/vt/vtgate/evalengine/abs.go

go/vt/vtgate/evalengine/abs_test.go

go/vt/vtgate/evalengine/abs.go

go/vt/vtgate/evalengine/internal/decimal/decimal.go

vmg · 2022-02-28T15:40:39Z

go/vt/vtgate/evalengine/func_test.go

+	"testing"
+)
+
+func Test_buildinAbs_call(t *testing.T) {


As I explained on my previous review, these are not good unit tests. They look like they've been auto-generated by an IDE, as the test table contains arguments that are always the same, and most puzzlingly, you're not actually verifying the output of the function you're testing!

You should instead focus on writing integration tests in integration/comparison_test.go. If those tests are comprehensive enough, this particular test will be superfluous.

Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> Remove stubbed validation errors Signed-off-by: notfelineit <[email protected]> Restyle page Signed-off-by: notfelineit <[email protected]> Refactor Signed-off-by: notfelineit <[email protected]> Add error state Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> Remove helper method Signed-off-by: notfelineit <[email protected]> Cleanup logs Signed-off-by: notfelineit <[email protected]> Undo accidental , unrelated changes to dynamic discovery Signed-off-by: notfelineit <[email protected]> Undo changes to env.sh Signed-off-by: notfelineit <[email protected]> Thread through vtadmin api Signed-off-by: notfelineit <[email protected]> Hook up UI to ValidateSchemaKeyspace Signed-off-by: notfelineit <[email protected]> Add comments to ValidateSchemaKeyspace Signed-off-by: notfelineit <[email protected]> Modify ValidateSchemaKeyspace to match old ValidateSchemaKeyspace in term of params Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> connect to vtadmin Signed-off-by: notfelineit <[email protected]> Fix nil pointer bug Signed-off-by: notfelineit <[email protected]> Fix linter issues Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> Add skipNoPrimary test Signed-off-by: notfelineit <[email protected]> Add GetVersion to vtctldServer and add test for ValidateVersionKeyspace Signed-off-by: notfelineit <[email protected]> Switch vtctlclient to use new version of ValidateVersionKeyspace Signed-off-by: notfelineit <[email protected]> Address PR comments Signed-off-by: notfelineit <[email protected]> Replace wrangler methods with vtctldserver replacements Signed-off-by: notfelineit <[email protected]> Fix type issues Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Alon Kenneth <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Alon Kenneth <[email protected]> Signed-off-by: akenneth <[email protected]> Signed-off-by: tianshishuo <[email protected]>

…rade build Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: tianshishuo <[email protected]>

In the original implementation, bidiStream used one channel, `errch`, to receive both mid-stream errors and "the stream is closed now" errors. This meant that, due to go's pseudo-random channel selection, if the stream was closed and there were still messages in the stream (so, both `ch` and `errch` were select-able), then there was a chance that an adapter's `Recv()` would return `io.EOF` even though there were still responses to pull off the stream. This version moves to a more context-cancel like paradigm, using a separate context to check if the stream is closed. Then, we adjust the template for `Recv` to select: 1. Is stream context done 2. Is streamclosed context done 4. Does mid-stream error channel have an error 3. Does message channel have a message And the template for `Send` to: 1. If streamclosed context done, return `errStreamClosed`; otherwise 2. Send to the message channel and return nil <hr/> Note that this commit also makes a more public/exported API for bidiStream, to set up a simpler (diff-wise) filemove in a later change. Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

…vt/vtctl` Turns out this construct is useful for writing stream-based unit tests for `grpcvtctldserver`, and this lets us reuse this code there without exporting it completely. Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

These were undocumented and broken, and they are no longer needed with the docker_local container being the preferred method for quick testing. Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

… command path Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

We already do this for BLOB and VARBINARY Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

…e server behavior by version Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Signed-off-by: tianshishuo <[email protected]>

vmg · 2022-03-01T13:07:16Z

There are, for some reason, 98 commits in this PR now. It looks like you screwed up a merge or a rebase. You'll need to fix this into (ideally) a single commit so we can review. Thanks!

tianshishuo requested review from harshit-gangal, systay and frouioui as code owners February 24, 2022 10:50

tianshishuo changed the title ~~Abs function add~~ Support built-in MySQL functions--abs() Feb 24, 2022

tianshishuo changed the title ~~Support built-in MySQL functions--abs()~~ Support built-in MySQL functions ABS Feb 24, 2022

vmg self-assigned this Feb 24, 2022

vmg requested changes Feb 24, 2022

View reviewed changes

go/vt/vtgate/evalengine/abs.go Outdated Show resolved Hide resolved

go/vt/vtgate/evalengine/abs.go Outdated Show resolved Hide resolved

go/vt/vtgate/evalengine/abs_test.go Outdated Show resolved Hide resolved

go/vt/vtgate/evalengine/abs.go Outdated Show resolved Hide resolved

vmg reviewed Feb 28, 2022

View reviewed changes

go/vt/vtgate/evalengine/internal/decimal/decimal.go Outdated Show resolved Hide resolved

vmg added the Component: Evalengine changes to the evaluation engine label Feb 28, 2022

vmg reviewed Feb 28, 2022

View reviewed changes

tianshishuo and others added 20 commits March 1, 2022 19:38

测试

4c9eb65

Signed-off-by: tianshishuo <[email protected]>

Add build in function(abs) for select

033330e

Signed-off-by: tianshishuo <[email protected]>

Add build in function(abs) for select

a5a0adc

Signed-off-by: tianshishuo <[email protected]>

Add build in function(abs) for select

b2fb7c0

Signed-off-by: tianshishuo <[email protected]>

Add build in function(abs) for select

e384b63

Signed-off-by: tianshishuo <[email protected]>

Add build in function(abs) for select

61d103a

Signed-off-by: tianshishuo <[email protected]>

Add Toggle component

25ce902

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Add VSchema validation to ValidateSchemaKeyspace

e052005

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Address PR comments after adding ValidateVSchema

4f666aa

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Fix vtctlclient methods to return errs if Results is not empty

035f2e5

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Have wrangler command print output

c18341a

Signed-off-by: notfelineit <[email protected]> Signed-off-by: tianshishuo <[email protected]>

docs: improve explanation of MySQLTopologyUser and associated params

3de555d

Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: tianshishuo <[email protected]>

add case for uint32 bind var + test

b6deb12

Signed-off-by: Alon Kenneth <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Throw an error on parse failure

14e47b8

Signed-off-by: Alon Kenneth <[email protected]> Signed-off-by: akenneth <[email protected]> Signed-off-by: tianshishuo <[email protected]>

test: of new get_latest_release script in query serving upgrade/downg…

b108431

…rade build Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: tianshishuo <[email protected]>

test: get latest release from origin instead of heads

c685fac

Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: tianshishuo <[email protected]>

ajm188 and others added 10 commits March 1, 2022 19:40

Switch from RWMutex to Mutex

7df1555

Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Remove unmaintained vagrant files

fa7345f

These were undocumented and broken, and they are no longer needed with the docker_local container being the preferred method for quick testing. Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Move deprecation warning to just before we actually invoke the legacy…

fbb6c82

… command path Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Add TEXT and BLOB fields to VDiff test table

8bb581f

Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Treat TEXT and VARCHAR fields the same way for comparisons

d514e7a

We already do this for BLOB and VARBINARY Signed-off-by: Matt Lord <[email protected]> Signed-off-by: tianshishuo <[email protected]>

Conn.ServerVersionAtLeast() and flavor.supportsFastDropTable(): defin…

e582330

…e server behavior by version Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: tianshishuo <[email protected]>

skip PURGE and EVAC states when fast DROP TABLE is supported

679b49e

Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: tianshishuo <[email protected]>

add builtin function abs

03e71e4

Signed-off-by: tianshishuo <[email protected]>

tianshishuo requested review from ajm188, doeg, notfelineit, deepthi, rohit-nayak-ps, shlomi-noach, GuptaManan100 and sougou as code owners March 1, 2022 11:41

vmg removed request for deepthi, systay, sougou, doeg, shlomi-noach, ajm188, harshit-gangal, notfelineit, frouioui, GuptaManan100 and rohit-nayak-ps March 1, 2022 13:07

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support built-in MySQL functions ABS #9774

Support built-in MySQL functions ABS #9774

tianshishuo commented Feb 24, 2022

vmg left a comment

vmg Feb 28, 2022

vmg commented Mar 1, 2022

Support built-in MySQL functions ABS #9774

Support built-in MySQL functions ABS #9774

Conversation

tianshishuo commented Feb 24, 2022

Description

Related Issue(s)

Checklist

Deployment Notes

vmg left a comment

Choose a reason for hiding this comment

vmg Feb 28, 2022

Choose a reason for hiding this comment

vmg commented Mar 1, 2022