Bug fixes: datasource format bug for bigquery, use UNION ALL instead …

…of just UNION, variationIdFormat not saving properly.
growthbook · Jul 21, 2021 · 503473f · 503473f · vercel · Jul 21, 2021
1 parent 0796406
commit 503473f
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 24 deletions.
diff --git a/packages/back-end/src/controllers/organizations.ts b/packages/back-end/src/controllers/organizations.ts
@@ -58,7 +58,6 @@ import { uploadFile } from "../services/files";
 import { ExperimentInterface } from "../../types/experiment";
 import { MetricModel } from "../models/MetricModel";
 import { MetricInterface } from "../../types/metric";
-import { format } from "sql-formatter";
 import { PostgresConnectionParams } from "../../types/integrations/postgres";
 import uniqid from "uniqid";
 import { WebhookModel } from "../models/WebhookModel";
@@ -997,15 +996,6 @@ export async function putDataSource(
     return;
   }
 
-  // Format queries on save
-  if (settings?.queries?.experimentsQuery) {
-    settings.queries.experimentsQuery = format(
-      settings.queries.experimentsQuery
-    );
-    settings.queries.usersQuery = format(settings.queries.usersQuery);
-    settings.queries.pageviewsQuery = format(settings.queries.pageviewsQuery);
-  }
-
   try {
     datasource.set("name", name);
     datasource.set("dateUpdated", new Date());

diff --git a/packages/back-end/src/integrations/SqlIntegration.ts b/packages/back-end/src/integrations/SqlIntegration.ts
@@ -14,7 +14,7 @@ import {
   VariationMetricResult,
   PastExperimentResult,
 } from "../types/Integration";
-import { format } from "sql-formatter";
+import { format, FormatOptions } from "sql-formatter";
 import { ExperimentPhase, ExperimentInterface } from "../../types/experiment";
 import { DimensionInterface } from "../../types/dimension";
 import { SegmentInterface } from "../../types/segment";
@@ -201,7 +201,8 @@ export default abstract class SqlIntegration
   }
 
   getPastExperimentQuery(from: Date) {
-    return format(`-- Past Experiments
+    return format(
+      `-- Past Experiments
     WITH
       __experiments as (
         ${getExperimentQuery(this.settings, this.getSchema())}
@@ -265,7 +266,9 @@ export default abstract class SqlIntegration
       -- Skip experiments that start of the very first day since we're likely missing data
       AND ${this.dateDiff(this.toTimestamp(from), "start_date")} > 2
     ORDER BY
-      experiment_id ASC, variation_id ASC`);
+      experiment_id ASC, variation_id ASC`,
+      this.getFormatOptions()
+    );
   }
   async runPastExperimentQuery(query: string): Promise<PastExperimentResult> {
     const rows: PastExperimentResponse = await this.runQuery(query);
@@ -287,7 +290,8 @@ export default abstract class SqlIntegration
     const userId = params.userIdType === "user";
 
     // TODO: support by date
-    return format(`-- ${params.name} - ${params.metric.name} Metric
+    return format(
+      `-- ${params.name} - ${params.metric.name} Metric
       WITH
         ${this.getIdentifiesCTE(userId, {
           segment: !!params.segmentQuery,
@@ -384,7 +388,7 @@ export default abstract class SqlIntegration
       ${
         params.includeByDate
           ? `
-        UNION SELECT
+        UNION ALL SELECT
           date,
           COUNT(*) as count,
           AVG(value) as mean,
@@ -405,13 +409,16 @@ export default abstract class SqlIntegration
       `
           : ""
       }
-      `);
+      `,
+      this.getFormatOptions()
+    );
   }
 
   getUsersQuery(params: UsersQueryParams): string {
     const userId = params.userIdType === "user";
 
-    return format(`-- ${params.name} - Number of Users
+    return format(
+      `-- ${params.name} - Number of Users
       WITH
         ${this.getIdentifiesCTE(userId, {
           segment: !!params.segmentQuery,
@@ -441,7 +448,7 @@ export default abstract class SqlIntegration
       ${
         params.includeByDate
           ? `
-        UNION SELECT
+        UNION ALL SELECT
           ${this.dateTrunc("u.actual_start")} as date,
           COUNT(DISTINCT u.user_id) as users
         FROM
@@ -458,7 +465,9 @@ export default abstract class SqlIntegration
       `
           : ""
       }
-      `);
+      `,
+      this.getFormatOptions()
+    );
   }
   async runUsersQuery(query: string): Promise<UsersResult> {
     const rows: UsersQueryResponse = await this.runQuery(query);
@@ -517,6 +526,12 @@ export default abstract class SqlIntegration
     return ret;
   }
 
+  getFormatOptions(): FormatOptions {
+    return {
+      language: "redshift",
+    };
+  }
+
   async getImpactEstimation(
     urlRegex: string,
     metric: MetricInterface,
@@ -572,8 +587,9 @@ export default abstract class SqlIntegration
     ]);
 
     const formatted =
-      [usersSql, metricSql, valueSql].map((sql) => format(sql)).join(";\n\n") +
-      ";";
+      [usersSql, metricSql, valueSql]
+        .map((sql) => format(sql, this.getFormatOptions()))
+        .join(";\n\n") + ";";
 
     if (
       users &&
@@ -938,7 +954,9 @@ export default abstract class SqlIntegration
 
     const results: ExperimentResults = {
       results: [],
-      query: query.map((q) => format(q)).join(";\n\n") + ";",
+      query:
+        query.map((q) => format(q, this.getFormatOptions())).join(";\n\n") +
+        ";",
     };
 
     dimensionMap.forEach((variations, k) => {

diff --git a/packages/front-end/components/Settings/EditDataSourceSettingsForm.tsx b/packages/front-end/components/Settings/EditDataSourceSettingsForm.tsx
@@ -171,10 +171,11 @@ const EditDataSourceSettingsForm: FC<{
                 setDatasource({
                   ...datasource,
                   settings: {
-                    variationIdFormat: e.target.value as "index" | "key",
                     ...datasource.settings,
+                    variationIdFormat: e.target.value as "index" | "key",
                   },
                 });
+                setDirty(true);
               }}
               required
               value={datasource.settings?.variationIdFormat || "index"}
@@ -329,10 +330,11 @@ FROM
                     setDatasource({
                       ...datasource,
                       settings: {
-                        variationIdFormat: e.target.value as "index" | "key",
                         ...datasource.settings,
+                        variationIdFormat: e.target.value as "index" | "key",
                       },
                     });
+                    setDirty(true);
                   }}
                   required
                   value={datasource.settings?.variationIdFormat || "index"}