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

builtin: add make_set built-in function #2831

Merged
merged 4 commits into from
Mar 17, 2017
Merged

Conversation

ariesdevil
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2017

CLA assistant check
All committers have signed the CLA.

if err != nil {
return types.Datum{}, errors.Trace(err)
}
d.SetString("")
Copy link
Member

Choose a reason for hiding this comment

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

if args[0].IsNull(), mysql will return null.
mysql> SELECT MAKE_SET(NULL,'a','b','c');
+----------------------------+
| MAKE_SET(NULL,'a','b','c') |
+----------------------------+
| NULL |
+----------------------------+

if args[i].IsNull() {
continue
}
if string(bitsValue[i-1]) == "1" {
Copy link
Member

Choose a reason for hiding this comment

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

It is better to use int bit-wise and shift operation. Not string compare.

@ariesdevil
Copy link
Author

@shenli fixed and plz review again.

@coocood
Copy link
Member

coocood commented Mar 15, 2017

LGTM
Thank you for your PR!

@ariesdevil ariesdevil force-pushed the dev branch 2 times, most recently from 2b463e6 to fb3c878 Compare March 16, 2017 08:00
@coocood
Copy link
Member

coocood commented Mar 17, 2017

Sorry for the delay, there are many PRs conflicting on typeinfer files.
Merging each one makes other PRs conflicting, so we have to queue the PRs.
when this PR is ready to merge, we will notify you to resolve conflicts.

@ariesdevil
Copy link
Author

OK, got it.

@tiancaiamao
Copy link
Contributor

The test could not reflect the reality, if you run the binary, you'll get this:

mysql> SELECT MAKE_SET(-100 | 4,'a','b','c');
ERROR 1105 (HY000): constant 18446744073709551516 overflows bigint

That's because, -100 | 4 will be calculate directly in test code, while it should be done in TiDB as an expression.

{[]interface{}{-100 | 4, "hello", "nice", "abc", "world"}, "abc,world"},

Okay, it involves another problem.

LGTM

@ariesdevil
Copy link
Author

@tiancaiamao I test on my machine and it works well (mysql version: 5.7.17):

mysql> SELECT MAKE_SET(-100 | 4,'a','b','c');
+--------------------------------+
| MAKE_SET(-100 | 4,'a','b','c') |
+--------------------------------+
| c                              |
+--------------------------------+
1 row in set (0.00 sec)

@zimulala zimulala merged commit 901040a into pingcap:master Mar 17, 2017
@ariesdevil ariesdevil deleted the dev branch March 17, 2017 05:22
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants