From 5e4cfafa9620b83c7f1b3bb33b8655d497a9a044 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 25 Oct 2024 11:53:27 +0100 Subject: [PATCH 1/3] Expose bigints as a column to do calculations on. --- .../buttons/grid/GridViewCalculationButton.svelte | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte b/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte index 72216f3b3b..db58ba4a8f 100644 --- a/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte +++ b/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte @@ -7,7 +7,12 @@ Icon, Multiselect, } from "@budibase/bbui" - import { CalculationType, canGroupBy, FieldType } from "@budibase/types" + import { + CalculationType, + canGroupBy, + FieldType, + isNumeric, + } from "@budibase/types" import InfoDisplay from "pages/builder/app/[application]/design/[screenId]/[componentId]/_components/Component/InfoDisplay.svelte" import { getContext } from "svelte" @@ -90,10 +95,7 @@ return Object.entries(schema) .filter(([field, fieldSchema]) => { // Only allow numeric fields that are not calculations themselves - if ( - fieldSchema.calculationType || - fieldSchema.type !== FieldType.NUMBER - ) { + if (fieldSchema.calculationType || !isNumeric(fieldSchema.type)) { return false } // Don't allow duplicates From f46a6684657e043fb6bac68d83f04603e7215cd5 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 25 Oct 2024 15:55:07 +0100 Subject: [PATCH 2/3] Allow bigints to be used in view calculations, and make sure they're accurate up to 64bit signed int via tests. --- packages/backend-core/src/sql/sql.ts | 65 +++++--- .../src/api/routes/tests/viewV2.spec.ts | 144 +++++++++++++----- .../src/utilities/rowProcessor/index.ts | 27 ++-- 3 files changed, 162 insertions(+), 74 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 88953bbf99..91dceaf5a6 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -179,12 +179,6 @@ class InternalBuilder { return this.table.schema[column] } - private supportsILike(): boolean { - return !( - this.client === SqlClient.ORACLE || this.client === SqlClient.SQL_LITE - ) - } - private quoteChars(): [string, string] { const wrapped = this.knexClient.wrapIdentifier("foo", {}) return [wrapped[0], wrapped[wrapped.length - 1]] @@ -216,8 +210,30 @@ class InternalBuilder { return formatter.wrap(value, false) } - private rawQuotedValue(value: string): Knex.Raw { - return this.knex.raw(this.quotedValue(value)) + private castIntToString(identifier: string | Knex.Raw): Knex.Raw { + switch (this.client) { + case SqlClient.ORACLE: { + return this.knex.raw("to_char(??)", [identifier]) + } + case SqlClient.POSTGRES: { + return this.knex.raw("??::TEXT", [identifier]) + } + case SqlClient.MY_SQL: + case SqlClient.MARIADB: { + return this.knex.raw("CAST(?? AS CHAR)", [identifier]) + } + case SqlClient.SQL_LITE: { + // Technically sqlite can actually represent numbers larger than a 64bit + // int as a string, but it does it using scientific notation (e.g. + // "1e+20") which is not what we want. Given that the external SQL + // databases are limited to supporting only 64bit ints, we settle for + // that here. + return this.knex.raw("printf('%d', ??)", [identifier]) + } + case SqlClient.MS_SQL: { + return this.knex.raw("CONVERT(NVARCHAR, ??)", [identifier]) + } + } } // Unfortuantely we cannot rely on knex's identifier escaping because it trims @@ -1078,21 +1094,26 @@ class InternalBuilder { query = query.count(`* as ${aggregation.name}`) } } else { - const field = `${tableName}.${aggregation.field} as ${aggregation.name}` - switch (op) { - case CalculationType.SUM: - query = query.sum(field) - break - case CalculationType.AVG: - query = query.avg(field) - break - case CalculationType.MIN: - query = query.min(field) - break - case CalculationType.MAX: - query = query.max(field) - break + const fieldSchema = this.getFieldSchema(aggregation.field) + if (!fieldSchema) { + // This should not happen in practice. + throw new Error( + `field schema missing for aggregation target: ${aggregation.field}` + ) } + + let aggregate = this.knex.raw("??(??)", [ + this.knex.raw(op), + this.rawQuotedIdentifier(`${tableName}.${aggregation.field}`), + ]) + + if (fieldSchema.type === FieldType.BIGINT) { + aggregate = this.castIntToString(aggregate) + } + + query = query.select( + this.knex.raw("?? as ??", [aggregate, aggregation.name]) + ) } } return query diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 2af11b513b..44e5875b7e 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -2268,58 +2268,118 @@ describe.each([ }) }) - describe("calculation views", () => { - it("should not remove calculation columns when modifying table schema", async () => { - let table = await config.api.table.save( - saveTableRequest({ - schema: { - name: { - name: "name", - type: FieldType.STRING, + !isLucene && + describe("calculation views", () => { + it("should not remove calculation columns when modifying table schema", async () => { + let table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + age: { + name: "age", + type: FieldType.NUMBER, + }, }, - age: { - name: "age", - type: FieldType.NUMBER, + }) + ) + + let view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + sum: { + visible: true, + calculationType: CalculationType.SUM, + field: "age", }, }, }) - ) - let view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - type: ViewV2Type.CALCULATION, - schema: { - sum: { - visible: true, - calculationType: CalculationType.SUM, - field: "age", + table = await config.api.table.get(table._id!) + await config.api.table.save({ + ...table, + schema: { + ...table.schema, + name: { + name: "name", + type: FieldType.STRING, + constraints: { presence: true }, + }, }, - }, + }) + + view = await config.api.viewV2.get(view.id) + expect(Object.keys(view.schema!).sort()).toEqual([ + "age", + "id", + "name", + "sum", + ]) }) - table = await config.api.table.get(table._id!) - await config.api.table.save({ - ...table, - schema: { - ...table.schema, - name: { - name: "name", - type: FieldType.STRING, - constraints: { presence: true }, - }, - }, - }) + describe("bigints", () => { + let table: Table + let view: ViewV2 - view = await config.api.viewV2.get(view.id) - expect(Object.keys(view.schema!).sort()).toEqual([ - "age", - "id", - "name", - "sum", - ]) + beforeEach(async () => { + table = await config.api.table.save( + saveTableRequest({ + schema: { + bigint: { + name: "bigint", + type: FieldType.BIGINT, + }, + }, + }) + ) + + view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + sum: { + visible: true, + calculationType: CalculationType.SUM, + field: "bigint", + }, + }, + }) + }) + + it("should not lose precision handling ints larger than JSs int53", async () => { + // The sum of the following 3 numbers cannot be represented by + // JavaScripts default int53 datatype for numbers, so this is a test + // that makes sure we aren't losing precision between the DB and the + // user. + await config.api.row.bulkImport(table._id!, { + rows: [ + { bigint: "1000000000000000000" }, + { bigint: "123" }, + { bigint: "321" }, + ], + }) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(1) + expect(rows[0].sum).toEqual("1000000000000000444") + }) + + it("should be able to handle up to 2**63 - 1 bigints", async () => { + await config.api.row.bulkImport(table._id!, { + rows: [{ bigint: "9223372036854775806" }, { bigint: "1" }], + }) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(1) + expect(rows[0].sum).toEqual("9223372036854775807") + }) + }) }) - }) }) describe("row operations", () => { diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index b9f4d0db8b..0ffa7600aa 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -440,19 +440,26 @@ export async function coreOutputProcessing( } if (sdk.views.isView(source)) { - const calculationFields = Object.keys( - helpers.views.calculationFields(source) - ) - - // We ensure all calculation fields are returned as numbers. During the + // We ensure calculation fields are returned as numbers. During the // testing of this feature it was discovered that the COUNT operation // returns a string for MySQL, MariaDB, and Postgres. But given that all - // calculation fields should be numbers, we blanket make sure of that - // here. - for (const key of calculationFields) { + // calculation fields (except ones operating on BIGINTs) should be + // numbers, we blanket make sure of that here. + for (const [name, field] of Object.entries( + helpers.views.calculationFields(source) + )) { + if ("field" in field) { + const targetSchema = table.schema[field.field] + // We don't convert BIGINT fields to floats because we could lose + // precision. + if (targetSchema.type === FieldType.BIGINT) { + continue + } + } + for (const row of rows) { - if (typeof row[key] === "string") { - row[key] = parseFloat(row[key]) + if (typeof row[name] === "string") { + row[name] = parseFloat(row[name]) } } } From 9ab266a96368fbd68666695931ad20517d780263 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 25 Oct 2024 16:38:24 +0100 Subject: [PATCH 3/3] Fix lint. --- .../buttons/grid/GridViewCalculationButton.svelte | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte b/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte index db58ba4a8f..87c77a1457 100644 --- a/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte +++ b/packages/builder/src/components/backend/DataTable/buttons/grid/GridViewCalculationButton.svelte @@ -7,12 +7,7 @@ Icon, Multiselect, } from "@budibase/bbui" - import { - CalculationType, - canGroupBy, - FieldType, - isNumeric, - } from "@budibase/types" + import { CalculationType, canGroupBy, isNumeric } from "@budibase/types" import InfoDisplay from "pages/builder/app/[application]/design/[screenId]/[componentId]/_components/Component/InfoDisplay.svelte" import { getContext } from "svelte"