Skip to content

[CALCITE-3539] Implement INSTR function#1946

Closed
xy2953396112 wants to merge 1 commit intoapache:masterfrom
xy2953396112:instr_functions
Closed

[CALCITE-3539] Implement INSTR function#1946
xy2953396112 wants to merge 1 commit intoapache:masterfrom
xy2953396112:instr_functions

Conversation

@xy2953396112
Copy link
Contributor

No description provided.

@jinxing64
Copy link
Contributor

jinxing64 commented Apr 26, 2020

Do we also need to update reference.md ?

@xy2953396112 xy2953396112 force-pushed the instr_functions branch 3 times, most recently from 8efc738 to 9406c0a Compare April 28, 2020 13:52
@xy2953396112
Copy link
Contributor Author

xy2953396112 commented Apr 28, 2020

Do we also need to update reference.md ?

Thanks, i have updated reference.md.

tester1.checkString("INSTR('foobarbar', 'bar')", "4", "INTEGER NOT NULL");
tester1.checkString("INSTR('bar', 'foobarbar')", "0", "INTEGER NOT NULL");
tester1.checkNull("INSTR('foobarbar', cast(null as varchar(1)))");
tester1.checkNull("INSTR(cast(null as varchar(1)), 'foobarbar')");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add negative tests?

| m | SPACE(integer) | Returns a string of *integer* spaces; returns an empty string if *integer* is less than 1
| o | SUBSTR(string, position [, substringLength ]) | Returns a portion of *string*, beginning at character *position*, *substringLength* characters long. SUBSTR calculates lengths using characters as defined by the input character set
| m | STRCMP(string, string) | Returns 0 if both of the strings are same and returns -1 when the first argument is smaller than the second and 1 when the second one is smaller the first one.
| m | INSTR(string, string) | Returns the index of the first occurrence of the second argument in the first argument and returns 0 if the first argument not include the second argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd better remove the period at the end of the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, STRCMP does the same, change it altogether

@DonnyZone
Copy link
Contributor

MySQL's document says: 'This function is multibyte safe, and is case-sensitive only if at least one argument is a binary string.'
It is better to add several tests for case-(in)sensitive.

| m | SPACE(integer) | Returns a string of *integer* spaces; returns an empty string if *integer* is less than 1
| o | SUBSTR(string, position [, substringLength ]) | Returns a portion of *string*, beginning at character *position*, *substringLength* characters long. SUBSTR calculates lengths using characters as defined by the input character set
| m | STRCMP(string, string) | Returns 0 if both of the strings are same and returns -1 when the first argument is smaller than the second and 1 when the second one is smaller the first one.
| m | INSTR(string, string) | Returns the index of the first occurrence of the second argument in the first argument and returns 0 if the first argument not include the second argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the description here and sort by alphabet.

@XuQianJin-Stars
Copy link
Contributor

hi @xy2953396112 There is an error in the title here, it should be the link https://issues.apache.org/jira/browse/CALCITE-3959

@zinking
Copy link
Contributor

zinking commented Jun 30, 2020

looks good overall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants