Repository: rickbergfalk/sqlpad
Pull request #854 (2020-10-19 18:08:31)

Remove schema level from Pinot connection schema response
Now that a `connections/<id>/schema` response fully supports returning a `tables` array without a `schemas` array, the Pinot driver can be updated to no longer return a stubbed in schema that doesn't  actually exist in Pinot.

The `formatSchemaQueryResults` function is also refactored and updated to send descriptions for tables and schema, as well as be overall more efficient with just 1 pass through the query result rows to construct the hierarchy.

In working on this implementation a few bugs were teased out of the autocomplete work in a prior PR, fixing mixed case table name support, as well as fixing keyword detection. 


Patch:
diff --git a/client/src/utilities/updateCompletions.ts b/client/src/utilities/updateCompletions.ts
index 471b75e6..8cc8f204 100644
--- a/client/src/utilities/updateCompletions.ts
+++ b/client/src/utilities/updateCompletions.ts
@@ -57,8 +57,6 @@ type AceCompletion = {
   // Greyed out text to show in prompt. Used by Ace
   meta: string;
   // pointers to parent objects if applicable
-  schemaId?: string;
-  tableId?: string;
   schemaCompletion?: AceCompletion;
   columnCompletions?: AceCompletion[];
 };
@@ -166,9 +164,11 @@ function updateCompletions(connectionSchema: ConnectionSchema) {
   }
 
   // Create a big regex for table patterns to find tables
-  const tableRegex = new RegExp(tablePatterns.join('|'), 'gi');
+  // This regex is /\b(table1|schema.table1|table2|schema.table2)\b/
+  // \b indicates a boundary, the parens and pipes mean "one of these values"
+  const tableRegex = new RegExp(`\\b(${tablePatterns.join('|')})\\b`, 'gi');
 
-  const keywordsRegex = /\bfrom|join|select|where|group|having|on\b/gi;
+  const keywordsRegex = /\b(from|join|select|where|group|having|on)\b/gi;
 
   const tableWantedKeywords = new Set(['from', 'join']);
   const columnWantedKeywords = new Set([
@@ -249,7 +249,7 @@ function updateCompletions(connectionSchema: ConnectionSchema) {
       // Try and derive based on basic matching
       // figure out if there are any schemas/tables referenced in query
       const allTokens: Set<string> = new Set(
-        session.getValue().match(tableRegex)
+        session.getValue().toLowerCase().match(tableRegex)
       );
 
       // First find any references of schemas or tables in tokens
diff --git a/server/drivers/pinot/index.js b/server/drivers/pinot/index.js
index af326657..f4369c9d 100644
--- a/server/drivers/pinot/index.js
+++ b/server/drivers/pinot/index.js
@@ -73,8 +73,6 @@ function testConnection(connection) {
 
 /**
  * Get schema for connection
- * TODO FIXME: PInot does not have a concept of schema, but SQLPad requires it at this time
- * SQLPad needs to be updated to support optional table_schema
  * @param {*} connection
  */
 async function getSchema(connection) {
@@ -87,7 +85,6 @@ async function getSchema(connection) {
     const schema = await pinot.getTableSchema(connection.controllerUrl, table);
     for (const dimension of schema.dimensionFieldSpecs) {
       columnRows.push({
-        table_schema: 'main',
         table_name: table,
         column_name: dimension.name,
         data_type: dimension.dataType,
@@ -96,7 +93,6 @@ async function getSchema(connection) {
     }
     for (const metric of schema.metricFieldSpecs) {
       columnRows.push({
-        table_schema: 'main',
         table_name: table,
         column_name: metric.name,
         data_type: metric.dataType,
diff --git a/server/drivers/utils.js b/server/drivers/utils.js
index edd32960..5566d127 100644
--- a/server/drivers/utils.js
+++ b/server/drivers/utils.js
@@ -1,39 +1,20 @@
-const _ = require('lodash');
-
-/**
- * Get an array of table objects from column rows for given schema
- * @param {Object[]} schemaTableColumnRows - rows of column-level data for tables of a given schema
- * @param {string} schemaTableColumnRows[].table_name - field containing table_name
- * @param {string} schemaTableColumnRows[].column_name - field containing column name
- * @param {string} schemaTableColumnRows[].data_type - field containing data type of column
- * @param {string} schemaTableColumnRows[].column_description - field containing description of column
- */
-function getTables(schemaTableColumnRows) {
-  const tables = [];
-  const byTableName = _.groupBy(schemaTableColumnRows, 'table_name');
-  for (const tableName in byTableName) {
-    if (byTableName.hasOwnProperty(tableName)) {
-      const tableObj = {
-        name: tableName,
-        // TODO populate table description?
-        description: '',
-        columns: byTableName[tableName].map((row) => {
-          return {
-            name: row.column_name,
-            description: row.column_description,
-            dataType: row.data_type,
-          };
-        }),
-      };
-      tables.push(tableObj);
-    }
-  }
-  return tables;
-}
-
 /**
- * Formats schema query results into
- * a nested map of objects representing schema tree
+ * Formats flat schema query results into a hierarchy
+ *
+ * Expected input is
+ * {
+ *   rows: [
+ *     {
+ *       schema_name: '',          // or table_schema. optional. name of schema if DB uses schemas
+ *       schema_description: '',   // description of schema. optional.
+ *       table_name: '',           // name of table.
+ *       table_description: '',    // description of table. optional.
+ *       column_name: '',          // name of column.
+ *       column_description: '',   // description of column. optional.
+ *       data_type: '',            // data type of column
+ *     }
+ *   ]
+ * }
  *
  * Returned format is
  * {
@@ -66,40 +47,89 @@ function formatSchemaQueryResults(queryResult) {
     return {};
   }
 
-  // queryResult row casing may not always be consistent with what is specified in query
-  // HANA is always uppercase despire aliasing as lower case for example
-  // To account for this loop through rows and normalize the case
-  const rows = queryResult.rows.map((row) => {
+  // IDs will be full paths of objects
+  // `schemaname`, `schemaname.tablename`
+  let hasSchema = false;
+  const schemasById = {};
+  const tablesById = {};
+
+  for (const row of queryResult.rows) {
+    // queryResult row casing may not always be consistent with what is specified in query
+    // HANA is always uppercase despire aliasing as lower case for example
+    // For example a row from HANA might be { TABLE_SCHEMA: 'name' } instead of { table_schema: 'name' } expected
+    // To account for this loop through rows and normalize the case
     const cleanRow = {};
     Object.keys(row).forEach((key) => {
       cleanRow[key.toLowerCase()] = row[key];
     });
-    return cleanRow;
-  });
 
-  const hasSchema = rows[0].hasOwnProperty('table_schema');
+    // Originally information schemas returned a field "table_schema" for schema name
+    // "schema_name" fits better and is more consistent
+    // This supports both fields, preferring schema_name if set;
+    const schemaName = cleanRow.schema_name || cleanRow.table_schema;
+    const schemaDescription = cleanRow.schema_description;
+    const tableName = cleanRow.table_name;
+    const tableDescription = cleanRow.table_description;
+    const columnName = cleanRow.column_name;
+    const dataType = cleanRow.data_type;
+    const columnDescription = cleanRow.column_description;
 
-  if (hasSchema) {
-    const tree = {
-      schemas: [],
-    };
-    const bySchema = _.groupBy(rows, 'table_schema');
-    for (const schema in bySchema) {
-      if (bySchema.hasOwnProperty(schema)) {
-        const schemaObj = {
-          name: schema,
-          // TODO populate schema description?
-          description: '',
-          tables: getTables(bySchema[schema]),
-        };
-        tree.schemas.push(schemaObj);
+    const schemaId = schemaName;
+    const tableId = schemaName ? `${schemaName}.${tableName}` : undefined;
+
+    // If schema exists and hasn't been added to index yet, add it
+    if (schemaId && !schemasById[schemaId]) {
+      hasSchema = true;
+      schemasById[schemaId] = {
+        name: schemaName,
+        description: schemaDescription,
+        tables: [],
+        // temporary index to make it efficient to add tables
+        tablesById: {},
+      };
+    }
+
+    // If table hasn't been captured in index yet, add it
+    if (!tablesById[tableId]) {
+      const table = {
+        name: tableName,
+        description: tableDescription,
+        columns: [],
+      };
+      tablesById[tableId] = table;
+
+      // If schema exists and table is not yet there, add it to schema
+      // Its pushed to tables array for final product, and added to tablesById map
+      // so final operation can remove tablesById map and not have to iterate the list again
+      if (schemaId && !schemasById[schemaId].tablesById[tableId]) {
+        schemasById[schemaId].tables.push(table);
+        schemasById[schemaId].tablesById[tableId] = table;
       }
     }
-    return tree;
+
+    const column = {
+      name: columnName,
+      description: columnDescription,
+      dataType,
+    };
+
+    tablesById[tableId].columns.push(column);
+  }
+
+  // If schema is present, loop over values and remove the tablesById
+  // The resulting array will be in the format needed
+  if (hasSchema) {
+    return {
+      schemas: Object.values(schemasById).map((schema) => {
+        delete schema.tablesById;
+        return schema;
+      }),
+    };
   }
 
+  // If there was no schema, tablesById has everything needed
   return {
-    tables: getTables(rows),
+    tables: Object.values(tablesById),
   };
 }
 


Test patch:
diff --git a/server/test/drivers/utils.js b/server/test/drivers/utils.js
new file mode 100644
index 00000000..164d6d95
--- /dev/null
+++ b/server/test/drivers/utils.js
@@ -0,0 +1,148 @@
+const assert = require('assert');
+const utils = require('../../drivers/utils');
+
+describe('drivers/utils', function () {
+  it('formatSchemaQueryResults handles full row set', function () {
+    const rows = [
+      {
+        schema_name: 's1',
+        schema_description: 's1d',
+        table_name: 't1',
+        table_description: 't1d',
+        column_name: 'c1',
+        column_description: 'c1d',
+        data_type: 'dt1',
+      },
+      {
+        schema_name: 's1',
+        schema_description: 's1d',
+        table_name: 't1',
+        table_description: 't1d',
+        column_name: 'c2',
+        column_description: 'c2d',
+        data_type: 'dt2',
+      },
+      {
+        schema_name: 's2',
+        schema_description: 's2d',
+        table_name: 't1',
+        table_description: 't1d',
+        column_name: 'c1',
+        column_description: 'c1d',
+        data_type: 'dt1',
+      },
+    ];
+
+    const res = utils.formatSchemaQueryResults({ rows });
+
+    assert.deepStrictEqual(res, {
+      schemas: [
+        {
+          name: 's1',
+          description: 's1d',
+          tables: [
+            {
+              name: 't1',
+              description: 't1d',
+              columns: [
+                {
+                  name: 'c1',
+                  description: 'c1d',
+                  dataType: 'dt1',
+                },
+                {
+                  name: 'c2',
+                  description: 'c2d',
+                  dataType: 'dt2',
+                },
+              ],
+            },
+          ],
+        },
+        {
+          name: 's2',
+          description: 's2d',
+          tables: [
+            {
+              name: 't1',
+              description: 't1d',
+              columns: [
+                {
+                  name: 'c1',
+                  description: 'c1d',
+                  dataType: 'dt1',
+                },
+              ],
+            },
+          ],
+        },
+      ],
+    });
+  });
+
+  it('formatSchemaQueryResults handles original row and mixed case', function () {
+    const rows = [
+      {
+        table_schema: 's1',
+        SCHEMA_DESCRIPTION: 's1d',
+        table_name: 't1',
+        table_description: 't1d',
+        column_name: 'c1',
+        column_description: 'c1d',
+        data_type: 'dt1',
+      },
+    ];
+
+    const res = utils.formatSchemaQueryResults({ rows });
+
+    assert.deepStrictEqual(res, {
+      schemas: [
+        {
+          name: 's1',
+          description: 's1d',
+          tables: [
+            {
+              name: 't1',
+              description: 't1d',
+              columns: [
+                {
+                  name: 'c1',
+                  description: 'c1d',
+                  dataType: 'dt1',
+                },
+              ],
+            },
+          ],
+        },
+      ],
+    });
+  });
+
+  it('formatSchemaQueryResults handles only required fields', function () {
+    const rows = [
+      {
+        table_name: 't1',
+        column_name: 'c1',
+        data_type: 'dt1',
+      },
+    ];
+
+    const res = utils.formatSchemaQueryResults({ rows });
+
+    assert.deepStrictEqual(res, {
+      tables: [
+        {
+          name: 't1',
+          description: undefined,
+          columns: [
+            {
+              name: 'c1',
+              description: undefined,
+              dataType: 'dt1',
+            },
+          ],
+        },
+      ],
+    });
+  });
+});