3959 instr #2

jhugomoore · 2023-04-27T18:07:39Z

No description provided.

tanclary

Nice job!!! Left some mainly minor comments, only other suggestion would be to add some quidem tests to BigQuery.iq, wouldn't say its mandatory, just an idea. Awesome first draft!

tanclary · 2023-04-27T18:48:11Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

-    final int from0 = from - 1; // 0-based
-    if (from0 > s.length() || from0 < 0) {
+    if (from == 0) {
+      throw new IllegalArgumentException("From position cannot be zero");


I think most exceptions thrown in this class are stored in the RESOURCES here. You could add an entry.

tanclary · 2023-04-27T18:50:09Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

-    final int from0 = from - 1;
-    if (from0 > s.length() || from0 < 0) {
+    if (from == 0) {
+      throw new IllegalArgumentException("From position cannot be zero");


same comment as above, could be useful just so this doesn't have to be retyped each time.

tanclary · 2023-04-27T18:59:09Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    if (from > 0) {
+      final int from0 = from - 1; // 0-based
+      if (from0 > s.length() || from0 < 0) {
+        return 0;


What happens if from0 == s.length()? Should this also return 0 since you can't get the 1-based (n+1)th position of a string of n characters? This would mean from > s.length(). Just curious

you're so right, great call

tanclary · 2023-04-27T19:02:01Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+        if (from == 0) {
+          return 0;
+        }
+        from -= (s.length() + 2);


Don't know if this could be helpful but it seems like there's a lot of checking to determine if FROM is a valid value or not, could a helper be useful? This logic is crazy so I don't know if it would help that much or not, just a thought.

yeah I think there's definitely some simplifying that could be done, especially with all the +1s -1s and the like flying around. I'll look into it

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

tanclary · 2023-04-27T19:08:49Z

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

+    final String query = "SELECT INSTR('A', 'ABC', 1, 1) from \"product\"";
+    final String expected = "SELECT INSTR('A', 'ABC', 1, 1)\n"
+        + "FROM foodmart.product";
+    final Sql sql = fixture().withBigQuery().withLibrary(SqlLibrary.BIG_QUERY);


think you can do something like sql(query).withBigQuery().withLibrary(SqlLibrary.BIG_QUERY).ok(expected); shouldn't need to create Sql object i dont think

also if the function is valid for other libraries, could be a good idea to test with those libraries as well

tanclary · 2023-04-27T19:10:20Z

core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java

+    assertThat(-1, is(position("f", "abcdec", 1, 1)));
+    assertThat(-1, is(position("c", "abcdec", 1, 3)));
+    try {
+      int i = position("c", "abcdec", 0, 1);


based testing 🙏

actually made me realize those tests were wrong lol, appreciate the comment

tanclary · 2023-04-27T19:10:34Z

site/_docs/reference.md

@@ -2693,6 +2693,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b o | GREATEST(expr [, expr ]*)                    | Returns the greatest of the expressions
 | b h s | IF(condition, value1, value2)              | Returns *value1* if *condition* is TRUE, *value2* otherwise
 | b | IFNULL(value1, value2)                         | Equivalent to `NVL(value1, value2)`
+| b o | INSTR(string1, string2[, int1, int2])        | return


just reminder to update this

tanclary · 2023-04-27T19:10:54Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

-  @Test void testLocalTimeFuncWithCurrentTime() {
-    testLocalTimeFunc(currentTimeString(LOCAL_TZ));
-  }
+//  @Tag("slow")


is this meant to be commented out?

oh lol oops, commented out cause it was slow when running tests, nice catch!

tanclary · 2023-04-27T19:12:43Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -6224,6 +6224,30 @@ private void testCurrentDateFunc(Pair<String, Hook.Closeable> pair) {
    f.checkNull("STRPOS(x'', null)");
  }

+  @Test void testInstrFunction() {
+    final SqlOperatorFixture f0 = fixture().setFor(SqlLibraryOperators.INSTR);
+


you should add a test that ensures the function doesnt work unless its set for bigquery, you should also test against each library the function supports, not just bigquery. good example here.

…acle library)

* Adds OAuth configs * Update configs

Add release notes. In the release notes for previous releases, change whitespace and formatting for consistency among releases, standardize on 'Postgres' rather than 'PostgreSQL', and move the 'Compatability' paragraph before the 'Breaking changes' section. Add users' aliases to .mailmap, so that the contributors list in the release notes contains people's real name. Fix a typo in FileReaderTest. Release candidate #1 introduced [CALCITE-6616] ClassNotFoundException: java.util.SequencedCollection at RelBuilder$Frame init The cause was compiling the release candidate using JDK 21, which caused the `interface SequencedCollection`, introduced in JDK 21, to be used in resolving methods. The fix (in release candidate #2) is to revert to JDK 8 for the release build (and revert the instructions in howto.md). We suspect that building with building with JDK 11 or 17 would also work. Revise template vote email. Fix the tag URL, add an end time for the vote, describe how to use 'code name (role)' vote format, sign using release manager's name. Close apache#3979

jhugomoore changed the base branch from main to joey April 27, 2023 18:08

Implemented INSTR function in SqlLibraryOperators

c4e49cf

jhugomoore force-pushed the 3959-INSTR branch 2 times, most recently from 39b2571 to aa66da6 Compare April 27, 2023 18:39

tanclary reviewed Apr 27, 2023

View reviewed changes

[CALCITE-3959] Add INSTR function (enabled in BigQuery, MySql, and Or…

dae97d8

…acle library)

jhugomoore force-pushed the 3959-INSTR branch 6 times, most recently from 8cf0c50 to 4179b7e Compare May 2, 2023 18:34

[CALCITE-3959] Add INSTR function (enabled in BigQuery, MySql, and Or…

2bf817b

…acle library)

jhugomoore force-pushed the 3959-INSTR branch from 4179b7e to 2bf817b Compare May 2, 2023 22:40

tanclary pushed a commit that referenced this pull request Nov 8, 2023

Add OAuth config files for use with Looker (#2)

a1e18a8

* Adds OAuth configs * Update configs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3959 instr #2

3959 instr #2

jhugomoore commented Apr 27, 2023

tanclary left a comment

tanclary Apr 27, 2023

tanclary Apr 27, 2023

tanclary Apr 27, 2023

jhugomoore Apr 27, 2023

tanclary Apr 27, 2023

jhugomoore Apr 27, 2023

tanclary Apr 27, 2023

tanclary Apr 27, 2023

tanclary Apr 27, 2023

jhugomoore Apr 27, 2023

tanclary Apr 27, 2023

jhugomoore Apr 27, 2023

tanclary Apr 27, 2023

jhugomoore Apr 27, 2023

tanclary Apr 27, 2023

3959 instr #2

Are you sure you want to change the base?

3959 instr #2

Conversation

jhugomoore commented Apr 27, 2023

tanclary 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

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