evalengine: Support built-in MySQL function for CONV function #11566

Weijun-H · 2022-10-24T16:43:27Z

Signed-off-by: Weijun-H [email protected]

Description

The following functions will be supported

CONV()

Related Issue(s)

evalengine: Support all built-in MySQL functions #9647

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Signed-off-by: Weijun-H <[email protected]>

vitess-bot · 2022-10-24T16:43:29Z

vmg

This is not a great implementation @Weijun-H. You need to put more thought into this. If you widen your set of integration test cases you'll see that it breaks at the seams very easily.

vmg · 2022-10-25T16:27:12Z

go/vt/vtgate/evalengine/string.go

+	fromBase := args[1]
+	toBase := args[2]


Capture the args by reference

vmg · 2022-10-25T16:27:44Z

go/vt/vtgate/evalengine/string.go

+	fromBase := args[1]
+	toBase := args[2]
+
+	if inarg.isNull() || fromBase.isNull() || toBase.isNull() || fromBase.int64() < 2 || fromBase.int64() > 36 || toBase.int64() < 2 || toBase.int64() > 36 {


You cannot call int64 over and over; toBase and fromBase need to be converted to their integer forms before they can be checked.

vmg · 2022-10-25T16:28:45Z

go/vt/vtgate/evalengine/string.go

+		return
+	}
+
+	inarg.makeSignedIntegral()


You don't want a signed integral for inarg. You want the raw textual bytes so you can process them.

vmg · 2022-10-25T16:30:47Z

go/vt/vtgate/evalengine/string.go

+
+	inarg.makeSignedIntegral()
+
+	num, _ := strconv.ParseInt(fmt.Sprint(inarg.int64()), int(fromBase.int64()), 64)


MySQL handles overflow with saturation, not 0-truncation.

Try:

+-----------------------------------------------------------------------------------------------------------------------+ | conv(99999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999, 10, 2) | +-----------------------------------------------------------------------------------------------------------------------+ | 1111111111111111111111111111111111111111111111111111111111111111 | +-----------------------------------------------------------------------------------------------------------------------+

vmg · 2022-10-25T16:31:43Z

go/vt/vtgate/evalengine/string.go

+	convNum := strconv.FormatUint(uint64(num), int(toBase.int64()))
+	convNum = strings.ToUpper(convNum)
+	result.setRaw(sqltypes.VarChar, []byte(convNum), inarg.collation())
+	result.makeTextual(env.DefaultCollation)


Absolutely no point on calling makeTextual after setRaw. Particularly if you're going to change the collation. You can set the desired collation directly on the previous call.

vmg · 2022-10-25T16:32:07Z

go/vt/vtgate/evalengine/string.go

+	_, f1 := args[0].typeof(env)
+	// typecheck the right-hand argument but ignore its flags
+	args[1].typeof(env)
+


Missing typecheck for args[2].

vmg · 2022-10-25T16:33:05Z

go/vt/vtgate/evalengine/integration/string_fun_test.go

+	cases := []string{
+		"10",
+		"10 + '10' + 10",
+		"-10",
+		"'10'",
+	}
+	bases := []string{
+		"-1",
+		"1",
+		"2",
+		"4",
+		"8",
+		"10",
+		"16",
+		"32",
+	}


This is a very limited set of test cases. You need to widen this. Particularly important: hex literals.

Signed-off-by: Weijun-H <[email protected]>

GuptaManan100

I actually checked '+10' and it fails.
I also checked and '10-9+10' also fails. You aren't supposed to evaluate the string in the conv function. Just use the raw bytes and interpret them in the given base. Your output for this string is 9 (which I don't understand how you got to anyways, 11 I could understand) but MySQL just says 10, because it ignores everything after the - sign.

GuptaManan100 · 2022-11-03T12:11:24Z

go/vt/vtgate/evalengine/integration/string_fun_test.go

+	cases := []string{
+		"-5.1",
+		"-5.9",
+		"0xa21 + '1'",
+		"-0xa21 + '1'",
+		"10",
+		"10 + '10' + 10",
+		"10 + '10' - 10",
+		"-10",
+		"'10'",
+		"10+'10'+'10a'+X'0a'",
+		"10 / 10",
+		"X'0FFFFFFFFFFFFFF'",
+		"99999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999",
+		"-99999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999",
+	}


These still aren't strong enough cases. What happens if we have a + in the beginning? What if we have 2 of those in the beginning? What if it is a string with a + and not a number?

GuptaManan100 · 2022-11-03T12:14:07Z

go/vt/vtgate/evalengine/string.go

+	if inarg.isNull() || inarg2.isNull() || inarg3.isNull() || fromBase < 2 || fromBase > 36 || toBase < 2 || toBase > 36 {
+		result.setNull()
+		return
+	}


I don't think the checks inarg2.isNull() or inarge3.isNull() is required. The values of fromBase and toBase would be 0 if they were null respectively, so the condition would still be true.

GuptaManan100 · 2022-11-03T12:19:57Z

go/vt/vtgate/evalengine/string.go

+	if t == sqltypes.Float64 {
+		for i, c := range rawString {
+			if c == '-' {
+				continue
+			}
+			if (fromBase <= 9 && c >= '0' && c <= rune('0'+fromBase)) || (fromBase > 9 && ((c >= '0' && c <= '9') || (c >= 'a' && c <= rune('a'+fromBase-9)))) {
+				continue
+			} else {
+				rawString = rawString[:i]
+				break
+			}
+		}
+	}


Is there any specific reason why you need the float type as a separate case?

GuptaManan100 · 2022-11-03T12:28:51Z

go/vt/vtgate/evalengine/string.go

+	}
+
+	re, _ := regexp.Compile(`[+-]?[0-9.x]+[a-vA-Vx]*`)
+	for _, num := range re.FindAllString(rawString, -1) {


Why do you need all substring matches? Don't you only need the first one? In what case would have more than 1 matches?

Signed-off-by: Weijun-H <[email protected]>

GuptaManan100

Apart from this, there are still cases that aren't checked and spoiler, some are still failing.
Specifically the case

 string_fun_test.go:256: different results: NULL; mysql response: VARCHAR("-H") (local collation: binary; mysql collation: utf8mb4_0900_ai_ci)
        query: SELECT CONV(-17, 10, -18) (SIMPLIFY=false)
    string_fun_test.go:252: different results: NULL; mysql response: VARCHAR("-15") (local collation: binary; mysql collation: utf8mb4_0900_ai_ci)
        query: SELECT CONV(-17, 16, -18) (SIMPLIFY=false)
    string_fun_test.go:256: different results: NULL; mysql response: VARCHAR("-15") (local collation: binary; mysql collation: utf8mb4_0900_ai_ci)
        query: SELECT CONV(-17, 16, -18) (SIMPLIFY=false)
    string_fun_test.go:252: different results: NULL; mysql response: VARCHAR("-23") (local collation: binary; mysql collation: utf8mb4_0900_ai_ci)
        query: SELECT CONV(-17, 32, -18) (SIMPLIFY=false)

MySQL allows to have negative from_base and to_base. If from_base is negative then the number is treated as a signed integer

GuptaManan100 · 2022-11-08T09:10:51Z

go/vt/vtgate/evalengine/string.go

+		toNum = strings.ToUpper(temp)
+	}
+
+	inarg.makeTextualAndConvert(env.DefaultCollation)


From what I can see, there really is no need to convert inarg to the environment's collation. You can just extract the collation from inarg and change the collation and then type cast the result directly using that.
Assuming that the goal is to have the collation finally be the default collation with the coercability and repertoire being inherited from the first argument.
In this case, you can even remove the inarg.makeUnsignedIntegral too, because you only need the raw bytes, so that is also only being done for the collation changes.

However, I don't know what coercability or repotoire we want. Is there some way to verify from MySQL what it does?

You can just extract the collation from inarg and change the collation and then type cast the result directly using that.

I have no idea how to do in this way.

GuptaManan100 · 2022-11-08T09:36:06Z

In my opinion, the best way to do the conversion is to do it in two parts. You should convert using from_base and handle the behaviour of a negative from_base value. Once that conversion is complete, then do a second conversion into the to_base value and also handle the negative values there.
From what I have observed from MySQL behaviour is that, if the from_base is negative, then your input is treated as a signed integer, so the limits of int64 apply. Otherwise, the limits of uint64 apply!

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(1000000000000000000000000000000,10,10);
+---------------------------------------------+
| CONV(1000000000000000000000000000000,10,10) |
+---------------------------------------------+
| 18446744073709551615                        |
+---------------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(1000000000000000000000000000000,-10,10);
+----------------------------------------------+
| CONV(1000000000000000000000000000000,-10,10) |
+----------------------------------------------+
| 9223372036854775807                          |
+----------------------------------------------+
1 row in set, 1 warning (0.00 sec)

and then if your to_base is negative, then your output can be signed -


mysql [localhost:8026] {msandbox} (test) > SELECT CONV(-1,-10,10);
+----------------------+
| CONV(-1,-10,10)      |
+----------------------+
| 18446744073709551615 |
+----------------------+
1 row in set (0.00 sec)

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(-1,-10,-10);
+------------------+
| CONV(-1,-10,-10) |
+------------------+
| -1               |
+------------------+
1 row in set (0.01 sec)

GuptaManan100 · 2022-11-08T09:39:02Z

These are all worthwhile test cases to have -

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(10000000000000000000000000,-10,-10);
+------------------------------------------+
| CONV(10000000000000000000000000,-10,-10) |
+------------------------------------------+
| 9223372036854775807                      |
+------------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(10000000000000000000000000,-10,10);
+-----------------------------------------+
| CONV(10000000000000000000000000,-10,10) |
+-----------------------------------------+
| 9223372036854775807                     |
+-----------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(10000000000000000000000000,10,10);
+----------------------------------------+
| CONV(10000000000000000000000000,10,10) |
+----------------------------------------+
| 18446744073709551615                   |
+----------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql [localhost:8026] {msandbox} (test) > SELECT CONV(10000000000000000000000000,10,-10);
+-----------------------------------------+
| CONV(10000000000000000000000000,10,-10) |
+-----------------------------------------+
| -1                                      |
+-----------------------------------------+
1 row in set, 1 warning (0.00 sec)

Signed-off-by: Weijun-H <[email protected]>

github-actions · 2022-12-11T01:32:21Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2022-12-19T01:31:39Z

This PR was closed because it has been stale for 7 days with no activity.

add support for CONV

88b3645

Signed-off-by: Weijun-H <[email protected]>

Weijun-H requested review from harshit-gangal, systay, frouioui and GuptaManan100 as code owners October 24, 2022 16:43

vmg requested changes Oct 25, 2022

View reviewed changes

Weijun-H added 2 commits November 1, 2022 17:24

failed to test hex literal

f1654a3

Signed-off-by: Weijun-H <[email protected]>

support hex literal

efbaff6

Signed-off-by: Weijun-H <[email protected]>

GuptaManan100 requested changes Nov 3, 2022

View reviewed changes

support more tests

67575f3

Signed-off-by: Weijun-H <[email protected]>

Weijun-H requested a review from GuptaManan100 November 5, 2022 13:45

GuptaManan100 reviewed Nov 8, 2022

View reviewed changes

support negetive base

654abcb

Signed-off-by: Weijun-H <[email protected]>

Weijun-H force-pushed the evalengine-conv branch from c6a8ede to 654abcb Compare November 9, 2022 15:29

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Dec 11, 2022

github-actions bot closed this Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evalengine: Support built-in MySQL function for CONV function #11566

evalengine: Support built-in MySQL function for CONV function #11566

Weijun-H commented Oct 24, 2022

vitess-bot bot commented Oct 24, 2022

vmg left a comment

vmg Oct 25, 2022

vmg Oct 25, 2022

vmg Oct 25, 2022

vmg Oct 25, 2022

vmg Oct 25, 2022

vmg Oct 25, 2022

vmg Oct 25, 2022

GuptaManan100 left a comment

GuptaManan100 Nov 3, 2022

GuptaManan100 Nov 3, 2022

GuptaManan100 Nov 3, 2022

GuptaManan100 Nov 3, 2022

GuptaManan100 left a comment

GuptaManan100 Nov 8, 2022

Weijun-H Nov 9, 2022

GuptaManan100 commented Nov 8, 2022

GuptaManan100 commented Nov 8, 2022

github-actions bot commented Dec 11, 2022

github-actions bot commented Dec 19, 2022


		inarg.makeSignedIntegral()

		num, _ := strconv.ParseInt(fmt.Sprint(inarg.int64()), int(fromBase.int64()), 64)

evalengine: Support built-in MySQL function for CONV function #11566

evalengine: Support built-in MySQL function for CONV function #11566

Conversation

Weijun-H commented Oct 24, 2022

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Oct 24, 2022

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

vmg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuptaManan100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuptaManan100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuptaManan100 commented Nov 8, 2022

GuptaManan100 commented Nov 8, 2022

github-actions bot commented Dec 11, 2022

github-actions bot commented Dec 19, 2022