You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2023/06/07 09:53:03 UTC
[GitHub] [ignite-3] zstan opened a new pull request, #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
zstan opened a new pull request, #2159:
URL: https://github.com/apache/ignite-3/pull/2159
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] lowka commented on a diff in pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159#discussion_r1223449044
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItFunctionsTest.java:
##########
@@ -285,16 +285,37 @@ public void testTypeOf() {
assertThrowsWithCause(() -> sql("SELECT TYPEOF(SELECT 1, 2)"), CalciteContextException.class);
}
+ /**
+ * Tests for {@code SUBSTRING(str, start[, length])} function.
+ */
+ @Test
+ public void testSubstring() {
+ assertQuery("SELECT SUBSTRING('1234567', 1, 3)").returns("123").check();
+ assertQuery("SELECT SUBSTRING('1234567', 2)").returns("234567").check();
+ assertQuery("SELECT SUBSTRING('1234567', -1)").returns("1234567").check();
+ assertQuery("SELECT SUBSTRING(1000, 1, 3)").returns("100").check();
+
+ assertQuery("SELECT SUBSTRING(NULL FROM 1 FOR 2)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('text' FROM 1 FOR null)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('test' FROM null FOR 2)").returns(null).check();
+
+ // uncomment after https://issues.apache.org/jira/browse/IGNITE-19686 was implemented.
+ //assertQuery("select SUBSTRING(s from i for l) from (values ('abc', null, 2)) as t (s, i, l);").returns(null).check();
+
+ assertThrowsWithCause(() -> sql("SELECT SUBSTRING('abcdefg', 1, -3)"), IgniteException.class, "negative substring length");
Review Comment:
@zstan Maybe we can also check that `SELECT SUBSTRING('abcdefg' FROM 1 FOR -1)` returns an error?
Apart from this + a tiny issue with a comment everything looks good.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/fun/IgniteSqlOperatorTable.java:
##########
@@ -103,10 +102,16 @@ public class IgniteSqlOperatorTable extends ReflectiveSqlOperatorTable {
* Generic {@code SUBSTR(string, position [, length]} function.
* This function works exactly the same as {@link SqlSubstringFunction SUSBSTRING(string, position [, length])}.
*/
- public static final SqlFunction SUBSTR = new SqlFunction("SUBSTR", SqlKind.OTHER_FUNCTION,
- ReturnTypes.ARG0_NULLABLE_VARYING, null,
- OperandTypes.STRING_INTEGER_OPTIONAL_INTEGER,
- SqlFunctionCategory.STRING);
+ public static final SqlFunction SUBSTRING = new SqlSubstringFunction();
Review Comment:
I think `SUBSTRING` and `SUBSTR`, since a comment over `SUBSTRING` belongs to `SUBSTR`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159#discussion_r1221658256
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItFunctionsTest.java:
##########
@@ -285,16 +285,37 @@ public void testTypeOf() {
assertThrowsWithCause(() -> sql("SELECT TYPEOF(SELECT 1, 2)"), CalciteContextException.class);
}
+ /**
+ * Tests for {@code SUBSTRING(str, start[, length])} function.
+ */
+ @Test
+ public void testSubstring() {
+ assertQuery("SELECT SUBSTRING('1234567', 1, 3)").returns("123").check();
+ assertQuery("SELECT SUBSTRING('1234567', 2)").returns("234567").check();
+ assertQuery("SELECT SUBSTRING('1234567', -1)").returns("1234567").check();
+ assertQuery("SELECT SUBSTRING(1000, 1, 3)").returns("100").check();
+
+ assertQuery("SELECT SUBSTRING(NULL FROM 1 FOR 2)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('text' FROM 1 FOR null)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('test' FROM null FOR 2)").returns(null).check();
+
+ // uncomment after https://issues.apache.org/jira/browse/CALCITE-5708 was merged.
Review Comment:
We don't track Calcite tickets.
Maybe there is an Ignite ticket to fix this issue or to bump Calcite dependency version?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159#issuecomment-1582326877
@lowka can u make a review plz ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159#discussion_r1221658256
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItFunctionsTest.java:
##########
@@ -285,16 +285,37 @@ public void testTypeOf() {
assertThrowsWithCause(() -> sql("SELECT TYPEOF(SELECT 1, 2)"), CalciteContextException.class);
}
+ /**
+ * Tests for {@code SUBSTRING(str, start[, length])} function.
+ */
+ @Test
+ public void testSubstring() {
+ assertQuery("SELECT SUBSTRING('1234567', 1, 3)").returns("123").check();
+ assertQuery("SELECT SUBSTRING('1234567', 2)").returns("234567").check();
+ assertQuery("SELECT SUBSTRING('1234567', -1)").returns("1234567").check();
+ assertQuery("SELECT SUBSTRING(1000, 1, 3)").returns("100").check();
+
+ assertQuery("SELECT SUBSTRING(NULL FROM 1 FOR 2)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('text' FROM 1 FOR null)").returns(null).check();
+ assertQuery("SELECT SUBSTRING('test' FROM null FOR 2)").returns(null).check();
+
+ // uncomment after https://issues.apache.org/jira/browse/CALCITE-5708 was merged.
Review Comment:
We don't track Calcite tickets.
Maybe there Ignite ticket to fix the issue or to bump Calcite dependency version?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov merged pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov merged PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] lowka commented on a diff in pull request #2159: IGNITE-19341 Sql. Correctly process null`s in SUBSTRING
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2159:
URL: https://github.com/apache/ignite-3/pull/2159#discussion_r1223442333
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/fun/IgniteSqlOperatorTable.java:
##########
@@ -103,10 +102,16 @@ public class IgniteSqlOperatorTable extends ReflectiveSqlOperatorTable {
* Generic {@code SUBSTR(string, position [, length]} function.
* This function works exactly the same as {@link SqlSubstringFunction SUSBSTRING(string, position [, length])}.
*/
- public static final SqlFunction SUBSTR = new SqlFunction("SUBSTR", SqlKind.OTHER_FUNCTION,
- ReturnTypes.ARG0_NULLABLE_VARYING, null,
- OperandTypes.STRING_INTEGER_OPTIONAL_INTEGER,
- SqlFunctionCategory.STRING);
+ public static final SqlFunction SUBSTRING = new SqlSubstringFunction();
Review Comment:
I think it would be better swap `SUBSTRING` and `SUBSTR`, since a comment over `SUBSTRING` belongs to `SUBSTR`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org