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

If every type of an expression is the same return that type as a result #4211

Closed

Conversation

AlecKazakova
Copy link
Collaborator

No description provided.

@AlecKazakova AlecKazakova force-pushed the astrong/2023-05-30/enable-encapsulating-type branch from 3dfd4f8 to d6f7c08 Compare May 30, 2023 18:23
@hfhbd
Copy link
Collaborator

hfhbd commented May 31, 2023

Do you have some public information what do you try to fix or change?

@shellderp
Copy link
Contributor

shellderp commented May 31, 2023

Yeah, I noticed that for MySQL, select max(timestamp) doesn't work, due to not allowing timestamp types in max(). Further, I noticed SELECT id*id doesn't work, due to being a MySQL BIGINT and not an ansi INTEGER. In both these cases there is only one unique type passed to encapsulatingType and we should just return that type.

However, I found a new example that doesn't work here,

CREATE TABLE `brand_orders` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `brand_shipping_subsidy_bps` bigint(20) NOT NULL DEFAULT '0',
  PRIMARY KEY (`id`)
);

CREATE TABLE `shipments` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `outgoing_cost_brand_minor` bigint(20) DEFAULT NULL,
  `faire_cost_brand_minor` bigint(20) DEFAULT NULL,
  PRIMARY KEY (`id`)
);

shippingSubsidiesSummaryForBrand:
SELECT
  bo.id AS brandOrderId,
  bo.brand_currency AS brandOrderCurrency,
  SUM((COALESCE(s.outgoing_cost_brand_minor, 0) + COALESCE(s.faire_cost_brand_minor, 0)) * bo.brand_shipping_subsidy_bps / 10000) AS paidByBrandCents
FROM brand_orders bo
  JOIN shipments s
;

(I'm still trying to simplify the repro case further)

This errors:

> Failed to compile /home/mgersh/code/sqldelight-fork/sqldelight-gradle-plugin/src/test/integration-mysql-schema/src/main/sqldelight/app/cash/sqldelight/mysql/integration/Dogs.sq:803:
       SUM((COALESCE(s.outgoing_cost_brand_minor, 0) + COALESCE(s.faire_cost_brand_minor, 0)) * bo.brand_shipping_subsidy_bps / 10000)

The reason is the types are slightly different

IntermediateType(dialectType=BIG_INT, javaType=kotlin.Long, column=null, name=value, bindArg=null, assumedCompatibleTypes=[], simplified=false)

vs

IntermediateType(dialectType=BIG_INT, javaType=kotlin.Long, column=ColumnDefMixin(COLUMN_DEF): [brand_orders : [id, brand_shipping_subsidy_bps [[rowid, oid, _rowid_]]]], name=brand_shipping_subsidy_bps, bindArg=null, assumedCompatibleTypes=[], simplified=false)]

We can instead check for distinct dialect type and this will work.

@shellderp
Copy link
Contributor

Actually I think this is fully broken. We're ignoring the passed in types list, so we're just returning the input type as the output type in many cases when it's not legal. So for example

"concat" -> encapsulatingType(exprList, TEXT)

Calling concat(5) will return INTEGER.

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 1, 2023

Yes, we don't have a real function representation which validates (and infers) the parameters, we only added support for the return type.

@shellderp
Copy link
Contributor

I think instead I will try to reimpl all the functions in Mysql type resolver to support mysql specific types.

For example,

"sum" -> {
  val type = exprList[0].type()
  if (type.dialectType == INTEGER && !type.javaType.isNullable) {
    type.asNullable()
  } else {
    IntermediateType(REAL).asNullable()
  }
}

this will check all other integer types like BIG_INT. Maybe there is a way to do this more generically, like telling the ansi resolver that INTEGER is analogous to BIG_INT? But for now, I will try to unblock our upgrade to 2.0 by redefining the functions for mysql.

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 1, 2023

What do you mean by upgrading, this did work in 1.5.5?

@shellderp
Copy link
Contributor

We are on 1.5.4 and the examples I gave worked. I am upgrading to 2.0 and it does not work (same story on latest master).

@shellderp
Copy link
Contributor

shellderp commented Jun 1, 2023

@hfhbd do you know if there is a reason we can't map BIG_INT to Ansi INTEGER? In ExprUtil.kt,

typeResolver.encapsulatingType(
  exprList = getExprList(),
  nullableIfAny = (
    this is SqlBinaryAddExpr || this is SqlBinaryMultExpr ||
      this is SqlBinaryPipeExpr
    ),
  INTEGER,
  REAL,
  TEXT,
  BLOB,
)

This used to handle multiplication for MySQL, but now it doesn't work since BIG_INT isn't in this list.

CREATE TABLE bro (
  id BIGINT(20) NOT NULL
);

dddd:
SELECT id*id FROM bro;
Caused by: java.lang.IllegalStateException: Failed to compile MySqlCompoundSelectStmtImpl(COMPOUND_SELECT_STMT): [] :
SELECT id*id FROM bro
	at app.cash.sqldelight.core.compiler.SqlDelightCompilerKt.tryWithElement(SqlDelightCompiler.kt:234)
	at app.cash.sqldelight.core.compiler.SqlDelightCompiler.writeQueryInterfaces(SqlDelightCompiler.kt:188)
	at app.cash.sqldelight.core.compiler.SqlDelightCompiler.writeQueryInterfaces$sqldelight_compiler(SqlDelightCompiler.kt:161)
	at app.cash.sqldelight.core.compiler.SqlDelightCompiler.writeInterfaces(SqlDelightCompiler.kt:51)
	at app.cash.sqldelight.core.SqlDelightEnvironment$generateSqlDelightFiles$2.invoke(SqlDelightEnvironment.kt:167)
	at app.cash.sqldelight.core.SqlDelightEnvironment$generateSqlDelightFiles$2.invoke(SqlDelightEnvironment.kt:158)
	at app.cash.sqldelight.core.SqlDelightEnvironment$forSourceFiles$1.invoke(SqlDelightEnvironment.kt:127)
	at app.cash.sqldelight.core.SqlDelightEnvironment$forSourceFiles$1.invoke(SqlDelightEnvironment.kt:122)
	at com.alecstrong.sql.psi.core.SqlCoreEnvironment.forSourceFiles$lambda$2(SqlCoreEnvironment.kt:180)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:240)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContentUnderDirectory(SqlCoreEnvironment.kt:237)
	at com.alecstrong.sql.psi.core.CoreFileIndex.iterateContent(SqlCoreEnvironment.kt:229)
	at com.alecstrong.sql.psi.core.SqlCoreEnvironment.forSourceFiles(SqlCoreEnvironment.kt:177)
	at app.cash.sqldelight.core.SqlDelightEnvironment.forSourceFiles(SqlDelightEnvironment.kt:122)
	at app.cash.sqldelight.core.SqlDelightEnvironment.forSqlFileBases(SqlDelightEnvironment.kt:368)
	at app.cash.sqldelight.core.SqlDelightEnvironment.generateSqlDelightFiles(SqlDelightEnvironment.kt:158)
	at app.cash.sqldelight.gradle.SqlDelightTask$GenerateInterfaces.execute(SqlDelightTask.kt:105)
	at org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:63)
	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:54)
	at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:48)
	at org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100)
	at org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:48)
	at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:49)
	at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:30)
	at org.gradle.workers.internal.IsolatedClassloaderWorkerFactory$1.lambda$execute$0(IsolatedClassloaderWorkerFactory.java:57)
	at org.gradle.workers.internal.AbstractWorker$1.call(AbstractWorker.java:44)
	at org.gradle.workers.internal.AbstractWorker$1.call(AbstractWorker.java:41)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:204)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:199)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:66)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:59)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:157)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:59)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.call(DefaultBuildOperationRunner.java:53)
	at org.gradle.internal.operations.DefaultBuildOperationExecutor.call(DefaultBuildOperationExecutor.java:73)
	at org.gradle.workers.internal.AbstractWorker.executeWrappedInBuildOperation(AbstractWorker.java:41)
	at org.gradle.workers.internal.IsolatedClassloaderWorkerFactory$1.execute(IsolatedClassloaderWorkerFactory.java:49)
	at org.gradle.workers.internal.DefaultWorkerExecutor.lambda$submitWork$0(DefaultWorkerExecutor.java:169)
	at org.gradle.internal.work.DefaultConditionalExecutionQueue$ExecutionRunner.runExecution(DefaultConditionalExecutionQueue.java:187)
	at org.gradle.internal.work.DefaultConditionalExecutionQueue$ExecutionRunner.access$700(DefaultConditionalExecutionQueue.java:120)
	at org.gradle.internal.work.DefaultConditionalExecutionQueue$ExecutionRunner$1.run(DefaultConditionalExecutionQueue.java:162)
	at org.gradle.internal.Factories$1.create(Factories.java:31)
	at org.gradle.internal.work.DefaultWorkerLeaseService.withLocks(DefaultWorkerLeaseService.java:249)
	at org.gradle.internal.work.DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:109)
	at org.gradle.internal.work.DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:114)
	at org.gradle.internal.work.DefaultConditionalExecutionQueue$ExecutionRunner.runBatch(DefaultConditionalExecutionQueue.java:157)
	at org.gradle.internal.work.DefaultConditionalExecutionQueue$ExecutionRunner.run(DefaultConditionalExecutionQueue.java:126)
	... 2 more
Caused by: java.lang.IllegalStateException: Type is null.
Types: [IntermediateType(dialectType=BIG_INT, javaType=kotlin.Long, column=ColumnDefMixin(COLUMN_DEF): [bro : [id [[rowid, oid, _rowid_]]]], name=id, bindArg=null, assumedCompatibleTypes=[], simplified=false)]
Sqltypes: [BIG_INT]
Typeorder: [Lapp.cash.sqldelight.dialect.api.DialectType;@1f136655
exprList: [SqlColumnExprImpl(COLUMN_EXPR): [bro : [id [[rowid, oid, _rowid_]]]], SqlColumnExprImpl(COLUMN_EXPR): [bro : [id [[rowid, oid, _rowid_]]]]]
	at app.cash.sqldelight.dialect.api.TypeResolverKt.encapsulatingType(TypeResolver.kt:75)
	at app.cash.sqldelight.core.lang.util.ExprUtilKt.ansiType(ExprUtil.kt:232)
	at app.cash.sqldelight.core.lang.util.ExprUtilKt.access$ansiType(ExprUtil.kt:1)
	at app.cash.sqldelight.core.lang.util.AnsiSqlTypeResolver.resolvedType(ExprUtil.kt:72)
	at app.cash.sqldelight.dialects.mysql.MySqlTypeResolver.resolvedType(MySqlTypeResolver.kt:35)
	at app.cash.sqldelight.core.lang.util.TreeUtilKt.type(TreeUtil.kt:83)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.type(NamedQuery.kt:224)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.typesExposed(NamedQuery.kt:218)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.resultColumns(NamedQuery.kt:78)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.access$resultColumns(NamedQuery.kt:51)
	at app.cash.sqldelight.core.compiler.model.NamedQuery$resultColumns$2.invoke(NamedQuery.kt:65)
	at app.cash.sqldelight.core.compiler.model.NamedQuery$resultColumns$2.invoke(NamedQuery.kt:63)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.getResultColumns(NamedQuery.kt:63)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.needsWrapper$sqldelight_compiler(NamedQuery.kt:136)
	at app.cash.sqldelight.core.compiler.model.NamedQuery.needsInterface$sqldelight_compiler(NamedQuery.kt:133)
	at app.cash.sqldelight.core.compiler.SqlDelightCompiler$writeQueryInterfaces$1$1.invoke(SqlDelightCompiler.kt:188)
	at app.cash.sqldelight.core.compiler.SqlDelightCompiler$writeQueryInterfaces$1$1.invoke(SqlDelightCompiler.kt:188)
	at app.cash.sqldelight.core.compiler.SqlDelightCompilerKt.tryWithElement(SqlDelightCompiler.kt:227)

(I added some extra log)

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 1, 2023

BigInt is 64 bit, mapped to kotlin.Long and Integer is 32 bit, mapped to kotlin.Int.

@shellderp
Copy link
Contributor

I don't think so

/**
 * Types which are retrieved the same way for all dialects.
 */
enum class PrimitiveType(override val javaType: TypeName) : DialectType {
  ARGUMENT(ANY.copy(nullable = true)),
  NULL(Nothing::class.asClassName().copy(nullable = true)),
  INTEGER(LONG),

INTEGER is Long

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 1, 2023

This is true for PrimitiveType.INTEGER, but MySql has its own Integer type: MySqlType.INTEGER:

@shellderp
Copy link
Contributor

But I'm asking if we can map MySQL.BIG_INT to PrimitiveType.INTEGER, I am not talking about MysqlType.INTEGER

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 1, 2023

Do you still want to write your own MySql dialect? If so, I guess (!) you could simple remove MySql.BIG_INT and replace it with PrimitiveType.INTEGER. Afaik this would be the simplest workaround.

@shellderp
Copy link
Contributor

Do you still want to write your own MySql dialect? If so, I guess (!) you could simple remove MySql.BIG_INT and replace it with PrimitiveType.INTEGER. Afaik this would be the simplest workaround.

I don't want to write my own, we already use the builtin one in this repo. I'm just wondering why this was changed for 2.0 since it's not obvious to me. Replacing it with PrimitiveType would be a workaround but we should probably support all mysql types for binary expressions like multiplication, I'll look at modifying the dialect

@AlecKazakova AlecKazakova deleted the astrong/2023-05-30/enable-encapsulating-type branch June 15, 2023 14:37
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.

3 participants