-
Notifications
You must be signed in to change notification settings - Fork 7
LabKey SQL hints and nits #7835
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
base: release26.7-SNAPSHOT
Are you sure you want to change the base?
Changes from all commits
0ee4507
5f7f493
1318557
68d6661
8abc1de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,10 +27,12 @@ | |
| import org.labkey.api.data.CompareType; | ||
| import org.labkey.api.data.Container; | ||
| import org.labkey.api.data.CoreSchema; | ||
| import org.labkey.api.data.DbScope; | ||
| import org.labkey.api.data.JdbcType; | ||
| import org.labkey.api.data.MethodInfo; | ||
| import org.labkey.api.data.MutableColumnInfo; | ||
| import org.labkey.api.data.SQLFragment; | ||
| import org.labkey.api.data.SqlSelector; | ||
| import org.labkey.api.data.TableInfo; | ||
| import org.labkey.api.data.dialect.SqlDialect; | ||
| import org.labkey.api.module.Module; | ||
|
|
@@ -167,7 +169,7 @@ public MethodInfo getMethodInfo() | |
| labkeyMethod.put("cos", new JdbcMethod("cos", JdbcType.DOUBLE, 1, 1)); | ||
| labkeyMethod.put("cot", new JdbcMethod("cot", JdbcType.DOUBLE, 1, 1)); | ||
| labkeyMethod.put("curdate", new JdbcMethod("curdate", JdbcType.DATE, 0, 0)); | ||
| labkeyMethod.put("curtime", new JdbcMethod("curtime", JdbcType.DATE, 0, 0)); | ||
| labkeyMethod.put("curtime", new JdbcMethod("curtime", JdbcType.TIME, 0, 0)); | ||
| labkeyMethod.put("dayofmonth", new JdbcMethod("dayofmonth", JdbcType.INTEGER, 1, 1)); | ||
| labkeyMethod.put("dayofweek", new JdbcMethod("dayofweek", JdbcType.INTEGER, 1, 1)); | ||
| labkeyMethod.put("dayofyear", new JdbcMethod("dayofyear", JdbcType.INTEGER, 1, 1)); | ||
|
|
@@ -1664,6 +1666,24 @@ public SQLFragment getSQL(SqlDialect dialect, SQLFragment[] arguments) | |
| } | ||
| }; | ||
| } | ||
|
|
||
| // "is distinct from" and "is not distinct from" operators in method form | ||
| labkeyMethod.put("is_distinct_from", new Method(JdbcType.BOOLEAN, 2, 2) | ||
| { | ||
| @Override | ||
| public MethodInfo getMethodInfo() | ||
| { | ||
| return new IsDistinctFromMethodInfo(IS); | ||
| } | ||
| }); | ||
| labkeyMethod.put("is_not_distinct_from", new Method(JdbcType.BOOLEAN, 2, 2) | ||
| { | ||
| @Override | ||
| public MethodInfo getMethodInfo() | ||
| { | ||
| return new IsDistinctFromMethodInfo(IS_NOT); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| final static Map<String, Method> postgresMethods = Collections.synchronizedMap(new CaseInsensitiveHashMap<>()); | ||
|
|
@@ -1843,24 +1863,6 @@ public SQLFragment getSQL(SqlDialect dialect, SQLFragment[] arguments) | |
| postgresMethods.put("jsonb_path_query_tz", new PassthroughMethod("jsonb_path_query_tz", JdbcType.VARCHAR, 2, 4)); | ||
| postgresMethods.put("jsonb_path_query_array_tz", new PassthroughMethod("jsonb_path_query_array_tz", JdbcType.VARCHAR, 2, 4)); | ||
| postgresMethods.put("jsonb_path_query_first_tz", new PassthroughMethod("jsonb_path_query_first_tz", JdbcType.VARCHAR, 2, 4)); | ||
|
|
||
| // "is distinct from" and "is not distinct from" operators in method form | ||
| labkeyMethod.put("is_distinct_from", new Method(JdbcType.BOOLEAN, 2, 2) | ||
| { | ||
| @Override | ||
| public MethodInfo getMethodInfo() | ||
| { | ||
| return new IsDistinctFromMethodInfo(IS); | ||
| } | ||
| }); | ||
| labkeyMethod.put("is_not_distinct_from", new Method(JdbcType.BOOLEAN, 2, 2) | ||
| { | ||
| @Override | ||
| public MethodInfo getMethodInfo() | ||
| { | ||
| return new IsDistinctFromMethodInfo(IS_NOT); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private static class IsDistinctFromMethodInfo extends AbstractMethodInfo | ||
|
|
@@ -1876,13 +1878,35 @@ private static class IsDistinctFromMethodInfo extends AbstractMethodInfo | |
| @Override | ||
| public SQLFragment getSQL(SqlDialect dialect, SQLFragment[] arguments) | ||
| { | ||
| SQLFragment a = arguments[0]; | ||
| SQLFragment b = arguments[1]; | ||
| SQLFragment ret = new SQLFragment(); | ||
| ret.append(" ((").append(arguments[0]).append(")"); | ||
| if (token == IS) | ||
| ret.append(" IS DISTINCT FROM "); | ||
|
|
||
| if (dialect.supportsNativeIsDistinctFrom()) | ||
| { | ||
| ret.append(" ((").append(a).append(")"); | ||
| if (token == IS) | ||
| ret.append(" IS DISTINCT FROM "); | ||
| else | ||
| ret.append(" IS NOT DISTINCT FROM "); | ||
| ret.append("(").append(b).append(")) "); | ||
| } | ||
| else | ||
| ret.append(" IS NOT DISTINCT FROM "); | ||
| ret.append("(").append(arguments[1]).append(")) "); | ||
| { | ||
| // "IS [NOT] DISTINCT FROM" isn't standard/portable SQL -- it's native only on PostgreSQL-family and | ||
| // Snowflake dialects. Elsewhere (e.g. SQL Server), rewrite as a CASE expression that always | ||
| // evaluates to a real TRUE/FALSE -- never NULL, even when exactly one side is null -- so it behaves | ||
| // the same as the native predicate would. | ||
| String notDistinctValue = token == IS ? dialect.getBooleanFALSE() : dialect.getBooleanTRUE(); | ||
| String distinctValue = token == IS ? dialect.getBooleanTRUE() : dialect.getBooleanFALSE(); | ||
|
|
||
| ret.append("(CASE WHEN (").append(a).append(") = (").append(b).append(")"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This (CASE WHEN) expression works as SELECT, but would it work in WHERE?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I'll double check. |
||
| ret.append(" OR ((").append(a).append(") IS NULL AND (").append(b).append(") IS NULL)"); | ||
| ret.append(" THEN ").append(notDistinctValue); | ||
| ret.append(" ELSE ").append(distinctValue); | ||
| ret.append(" END)"); | ||
| } | ||
|
|
||
| return ret; | ||
| } | ||
| } | ||
|
|
@@ -2013,5 +2037,42 @@ public void testSimpleString() | |
| assertNotSimpleString(new SQLFragment("SELECT 'test'")); | ||
| assertNotSimpleString(new SQLFragment("'test''string'")); | ||
| } | ||
|
|
||
| // Exercises both the native and portable-fallback branches of IsDistinctFromMethodInfo.getSQL() against every | ||
| // dialect that's actually connected in this environment, not just whichever dialect the current CI leg happens | ||
| // to be running against. A FROM-less SELECT works everywhere except Oracle, which requires FROM DUAL. | ||
| @Test | ||
| public void testIsDistinctFrom() | ||
| { | ||
| record Case(String a, String b, boolean distinct) {} | ||
| List<Case> cases = List.of( | ||
| new Case("1", "2", true), | ||
| new Case("1", "1", false), | ||
| new Case("NULL", "NULL", false), | ||
| new Case("1", "NULL", true) | ||
| ); | ||
|
|
||
| for (DbScope scope : DbScope.getDbScopesToTest()) | ||
| { | ||
| SqlDialect d = scope.getSqlDialect(); | ||
|
|
||
| for (Case c : cases) | ||
| { | ||
| assertIsDistinctFrom(scope, d, IS, c.a(), c.b(), c.distinct()); | ||
| assertIsDistinctFrom(scope, d, IS_NOT, c.a(), c.b(), !c.distinct()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void assertIsDistinctFrom(DbScope scope, SqlDialect d, int token, String a, String b, boolean expected) | ||
| { | ||
| SQLFragment expr = new IsDistinctFromMethodInfo(token).getSQL(d, new SQLFragment[]{new SQLFragment(a), new SQLFragment(b)}); | ||
| SQLFragment sql = new SQLFragment("SELECT ").append(expr); | ||
| if (d.isOracle()) | ||
| sql.append(" FROM DUAL"); | ||
|
|
||
| Boolean result = new SqlSelector(scope, sql).getObject(Boolean.class); | ||
| assertEquals(d.getClass().getSimpleName() + ": " + sql.toDebugString(), expected, result); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this applied from a patch? They should be added around line 546 instead.