(core) Linking summary tables grouped by list columns

Summary:
Prefix keys of `LinkingState.filterColValues` with `_contains:` when the source column is a ChoiceList or ReferenceList.

This is parsed out to make a boolean `isContainsFilter` which is kept in each value of `QueryRefs.filterTuples` (previously `filterPairs`).

Then when converting back in `convertQueryFromRefs` we construct `Query.contains: {[colId: string]: boolean}`.

Finally `getFilterFunc` uses `Query.contains` to decide what kind of filtering to do.

This is not pretty, but the existing code is already very complex and it was hard to find something that wouldn't require touching loads of code just to make things compile.

Test Plan: Added a new nbrowser test and fixture, tests that selecting a source table by summary tables grouped by a choicelist column, non-list column, and both all filter the correct data.

Reviewers: dsagal

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D2940

app/client/components/BaseView.js

@@ -26,6 +26,7 @@ const {copyToClipboard} = require('app/client/lib/copyToClipboard');
 const {setTestState} = require('app/client/lib/testState');
 const {ExtraRows} = require('app/client/models/DataTableModelWithDiff');
 const {createFilterMenu} = require('app/client/ui/ColumnFilterMenu');
+const {encodeObject} = require("app/plugin/objtypes");
 
 /**
  * BaseView forms the basis for ViewSection classes.
@@ -140,7 +141,12 @@ function BaseView(gristDoc, viewSectionModel, options) {
 
   this._linkingFilter = this.autoDispose(ko.computed(() => {
     const linking = this._linkingState();
-    return linking && linking.filterColValues ? linking.filterColValues() : {};
+    const result = linking && linking.filterColValues ? linking.filterColValues() : {filters: {}};
+    result.operations = result.operations || {};
+    for (const key in result.filters) {
+      result.operations[key] = result.operations[key] || 'in';
+    }
+    return result;
   }));
 
   // A computed for the rowId of the row selected by section linking.
@@ -204,7 +210,8 @@ function BaseView(gristDoc, viewSectionModel, options) {
   // dependency changes.
   this.autoDispose(ko.computed(() => {
     this._isLoading(true);
-    this._queryRowSource.makeQuery(this._linkingFilter(), (err) => {
+    const linkingFilter = this._linkingFilter();
+    this._queryRowSource.makeQuery(linkingFilter.filters, linkingFilter.operations, (err) => {
       if (this.isDisposed()) { return; }
       if (err) { window.gristNotify(`Query error: ${err.message}`); }
       this.onTableLoaded();
@@ -373,8 +380,11 @@ BaseView.prototype._parsePasteForView = function(data, cols) {
 };
 
 BaseView.prototype._getDefaultColValues = function() {
-  const filterValues = this._linkingFilter.peek();
+  const {filters, operations} = this._linkingFilter.peek();
-  return _.mapObject(_.pick(filterValues, v => (v.length > 0)), v => v[0]);
+  return _.mapObject(
+      _.pick(filters, v => (v.length > 0)),
+      (value, key) => operations[key] === "intersects" ? encodeObject(value) : value[0]
+  );
 };
 
 /**

app/client/components/LinkingState.js

@@ -77,7 +77,7 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl
             }
           }
         }
-        return {[tgtColId]: Array.from(srcValues)};
+        return {filters: {[tgtColId]: Array.from(srcValues)}};
       }));
     } else if (srcColId) {
       let srcRowModel = this.autoDispose(srcTableModel.createFloatingRowModel());
@@ -88,13 +88,13 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl
         this.filterColValues = this.autoDispose(ko.computed(() => {
           const srcRowId = srcSection.activeRowId();
           srcRowModel.assign(srcRowId);
-          return {[tgtColId]: [srcCell()]};
+          return {filters: {[tgtColId]: [srcCell()]}};
         }));
       }
     } else {
       this.filterColValues = this.autoDispose(ko.computed(() => {
         const srcRowId = srcSection.activeRowId();
-        return {[tgtColId]: [srcRowId]};
+        return {filters: {[tgtColId]: [srcRowId]}};
       }));
     }
   } else if (isSummaryOf(srcSection.table(), tgtSection.table())) {
@@ -103,17 +103,24 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl
     // those in the srcSection).
     // TODO: This approach doesn't help cursor-linking (the other direction). If we have the
     // inverse of summary-table's 'group' column, we could implement both, and more efficiently.
+    const isDirectSummary = srcSection.table().summarySourceTable() === tgtSection.table().getRowId();
     this.filterColValues = this.autoDispose(ko.computed(() => {
       const srcRowId = srcSection.activeRowId();
-      const filter = {};
+      const filters = {};
-      for (const c of srcSection.table().columns().all()) {
+      const operations = {};
-        if (c.summarySourceCol()) {
+      for (const c of srcSection.table().groupByColumns()) {
-          const colId = c.summarySource().colId();
+        const col = c.summarySource();
-          const srcValue = srcTableData.getValue(srcRowId, colId);
+        const colId = col.colId();
-          filter[colId] = [srcValue];
+        const srcValue = srcTableData.getValue(srcRowId, colId);
+        filters[colId] = [srcValue];
+        if (isDirectSummary) {
+          const tgtColType = col.type();
+          if (tgtColType === 'ChoiceList' || tgtColType.startsWith('RefList:')) {
+            operations[colId] = 'intersects';
+          }
         }
       }
-      return filter;
+      return {filters, operations};
     }));
   } else if (isSummaryOf(tgtSection.table(), srcSection.table())) {
     // TODO: We should move the cursor, but don't currently it for summaries. For that, we need a

app/client/models/QuerySet.ts

@@ -30,8 +30,8 @@ import * as DataTableModel from 'app/client/models/DataTableModel';
 import {DocModel} from 'app/client/models/DocModel';
 import {BaseFilteredRowSource, RowId, RowList, RowSource} from 'app/client/models/rowset';
 import {TableData} from 'app/client/models/TableData';
-import {ActiveDocAPI, Query} from 'app/common/ActiveDocAPI';
+import {ActiveDocAPI, ClientQuery, QueryOperation} from 'app/common/ActiveDocAPI';
-import {TableDataAction} from 'app/common/DocActions';
+import {CellValue, TableDataAction} from 'app/common/DocActions';
 import {DocData} from 'app/common/DocData';
 import {nativeCompare} from 'app/common/gutil';
 import {IRefCountSub, RefCountMap} from 'app/common/RefCountMap';
@@ -41,6 +41,8 @@ import {tbind} from 'app/common/tbind';
 import {Disposable, Holder, IDisposableOwnerT} from 'grainjs';
 import * as ko from 'knockout';
 import debounce = require('lodash/debounce');
+import {isList} from "app/common/gristTypes";
+import {decodeObject} from "app/plugin/objtypes";
 
 // Limit on the how many rows to request for OnDemand tables.
 const ON_DEMAND_ROW_LIMIT = 10000;
@@ -54,9 +56,11 @@ const MAX_SQL_PARAMS = 500;
  */
 export interface QueryRefs {
   tableRef: number;
-  filterPairs: Array<[number, any[]]>;
+  filterTuples: Array<FilterTuple>;
 }
 
+type FilterTuple = [number, QueryOperation, any[]];
+
 /**
  * QuerySetManager keeps track of all queries for a GristDoc instance. It is also responsible for
  * disposing all state associated with queries when a GristDoc is disposed.
@@ -83,7 +87,7 @@ export class QuerySetManager extends Disposable {
     }));
   }
 
-  public useQuerySet(owner: IDisposableOwnerT<IRefCountSub<QuerySet>>, query: Query): QuerySet {
+  public useQuerySet(owner: IDisposableOwnerT<IRefCountSub<QuerySet>>, query: ClientQuery): QuerySet {
     // Convert the query to a string key which identifies it.
     const queryKey: string = encodeQuery(convertQueryToRefs(this._docModel, query));
 
@@ -150,8 +154,10 @@ export class DynamicQuerySet extends RowSource {
    * argument to cb() is true if any data was changed, and false if not. Note that for a series of
    * makeQuery() calls, cb() is always called at least once, and always asynchronously.
    */
-  public makeQuery(filters: {[colId: string]: any[]}, cb: (err: Error|null, changed: boolean) => void): void {
+  public makeQuery(filters: {[colId: string]: any[]},
-    const query: Query = {tableId: this._tableModel.tableData.tableId, filters};
+                   operations: {[colId: string]: QueryOperation},
+                   cb: (err: Error|null, changed: boolean) => void): void {
+    const query: ClientQuery = {tableId: this._tableModel.tableData.tableId, filters, operations};
     const newQuerySet = this._querySetManager.useQuerySet(this._holder, query);
 
     // CB should be called asynchronously, since surprising hard-to-debug interactions can happen
@@ -200,7 +206,7 @@ export class QuerySet extends BaseFilteredRowSource {
 
   constructor(docModel: DocModel, docComm: ActiveDocAPI, queryKey: string, qsm: QuerySetManager) {
     const queryRefs: QueryRefs = decodeQuery(queryKey);
-    const query: Query = convertQueryFromRefs(docModel, queryRefs);
+    const query: ClientQuery = convertQueryFromRefs(docModel, queryRefs);
 
     super(getFilterFunc(docModel.docData, query));
     this.isTruncated = false;
@@ -296,22 +302,34 @@ export class TableQuerySets {
 /**
  * Returns a filtering function which tells whether a row matches the given query.
  */
-export function getFilterFunc(docData: DocData, query: Query): RowFilterFunc<RowId> {
+export function getFilterFunc(docData: DocData, query: ClientQuery): RowFilterFunc<RowId> {
   // NOTE we rely without checking on tableId and colIds being valid.
   const tableData: BaseTableData = docData.getTable(query.tableId)!;
-  const colIds = Object.keys(query.filters).sort();
+  const colFuncs = Object.keys(query.filters).sort().map(
-  const colPairs = colIds.map(
+    (colId) => {
-    (c) => [tableData.getRowPropFunc(c)!, new Set(query.filters[c])] as [RowPropFunc, Set<any>]);
+      const getter = tableData.getRowPropFunc(colId)!;
-  return (rowId: RowId) => colPairs.every(([getter, values]) => values.has(getter(rowId)));
+      const values = new Set(query.filters[colId]);
+      switch (query.operations![colId]) {
+        case "intersects":
+          return (rowId: RowId) => {
+            const value = getter(rowId) as CellValue;
+            return isList(value) &&
+              (decodeObject(value) as unknown[]).some(v => values.has(v));
+          };
+        case "in":
+          return (rowId: RowId) => values.has(getter(rowId));
+        default:
+          throw new Error("Unknown operation");
+      }
+    });
+  return (rowId: RowId) => colFuncs.every(f => f(rowId));
 }
 
-type RowPropFunc = (rowId: RowId) => any;
-
 /**
  * Helper that converts a Query (with tableId/colIds) to an object with tableRef/colRefs (i.e.
  * rowIds), and consistently sorted. We use that to identify a Query across table/column renames.
  */
-function convertQueryToRefs(docModel: DocModel, query: Query): QueryRefs {
+function convertQueryToRefs(docModel: DocModel, query: ClientQuery): QueryRefs {
   const tableRec: any = docModel.dataTables[query.tableId].tableMetaRow;
 
   const colRefsByColId: {[colId: string]: number} = {};
@@ -319,26 +337,32 @@ function convertQueryToRefs(docModel: DocModel, query: Query): QueryRefs {
     colRefsByColId[col.colId.peek()] = col.getRowId();
   }
 
-  const colIds = Object.keys(query.filters);
+  const filterTuples = Object.keys(query.filters).map((colId) => {
-  const filterPairs = colIds.map((c) => [colRefsByColId[c], query.filters[c]] as [number, any]);
+    const values = query.filters[colId];
+    // Keep filter values sorted by value, for consistency.
+    values.sort(nativeCompare);
+    return [colRefsByColId[colId], query.operations![colId], values] as FilterTuple;
+  });
   // Keep filters sorted by colRef, for consistency.
-  filterPairs.sort((a, b) => nativeCompare(a[0], b[0]));
+  filterTuples.sort((a, b) =>
-  // Keep filter values sorted by value, for consistency.
+    nativeCompare(a[0], b[0]) || nativeCompare(a[1], b[1]));
-  filterPairs.forEach(([colRef, values]) => values.sort(nativeCompare));
+  return {tableRef: tableRec.getRowId(), filterTuples};
-  return {tableRef: tableRec.getRowId(), filterPairs};
 }
 
 /**
  * Helper to convert a QueryRefs (using tableRef/colRefs) object back to a Query (using
  * tableId/colIds).
  */
-function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): Query {
+function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): ClientQuery {
   const tableRec = docModel.dataTablesByRef.get(queryRefs.tableRef)!.tableMetaRow;
   const filters: {[colId: string]: any[]} = {};
-  for (const [colRef, values] of queryRefs.filterPairs) {
+  const operations: {[colId: string]: QueryOperation} = {};
-    filters[docModel.columns.getRowModel(colRef).colId.peek()] = values;
+  for (const [colRef, operation, values] of queryRefs.filterTuples) {
+    const colId = docModel.columns.getRowModel(colRef).colId.peek();
+    filters[colId] = values;
+    operations[colId] = operation;
   }
-  return {tableId: tableRec.tableId.peek(), filters};
+  return {tableId: tableRec.tableId.peek(), filters, operations};
 }
 
 /**
@@ -349,13 +373,13 @@ function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): Query {
  * guaranteed. This is important to produce consistent results (same query => same encoding).
  */
 function encodeQuery(queryRefs: QueryRefs): string {
-  return JSON.stringify([queryRefs.tableRef, queryRefs.filterPairs]);
+  return JSON.stringify([queryRefs.tableRef, queryRefs.filterTuples]);
 }
 
 // Decode an encoded QueryRefs.
 function decodeQuery(queryKey: string): QueryRefs {
-  const [tableRef, filterPairs] = JSON.parse(queryKey);
+  const [tableRef, filterTuples] = JSON.parse(queryKey);
-  return {tableRef, filterPairs};
+  return {tableRef, filterTuples};
 }
 
 /**
@@ -364,7 +388,7 @@ function decodeQuery(queryKey: string): QueryRefs {
  */
 function makeQueryInvalidComputed(docModel: DocModel, queryRefs: QueryRefs): ko.Computed<boolean> {
   const tableFlag: ko.Observable<boolean> = docModel.tables.getRowModel(queryRefs.tableRef)._isDeleted;
-  const colFlags: Array<ko.Observable<boolean>> = queryRefs.filterPairs.map(
+  const colFlags: Array<ko.Observable<boolean>> = queryRefs.filterTuples.map(
-    ([colRef, values]) => docModel.columns.getRowModel(colRef)._isDeleted);
+    ([colRef, , ]) => docModel.columns.getRowModel(colRef)._isDeleted);
   return ko.computed(() => Boolean(tableFlag() || colFlags.some((c) => c())));
 }

app/common/ActiveDocAPI.ts

@@ -62,16 +62,33 @@ export interface ImportTableResult {
  *    {tableId: "Projects", filters: {}}
  *    {tableId: "Employees", filters: {Status: ["Active"], Dept: ["Sales", "HR"]}}
  */
-export interface Query {
+interface BaseQuery {
   tableId: string;
   filters: {
     [colId: string]: any[];
   };
+}
 
+/**
+ * Query that can only be used on the client side.
+ * Allows filtering with more complex operations.
+ */
+export interface ClientQuery extends BaseQuery {
+  operations?: {
+    [colId: string]: QueryOperation;
+  }
+}
+
+/**
+ * Query intended to be sent to a server.
+ */
+export interface ServerQuery extends BaseQuery {
   // Queries to server for onDemand tables will set a limit to avoid bringing down the browser.
   limit?: number;
 }
 
+export type QueryOperation = "in" | "intersects";
+
 /**
  * Response from useQuerySet(). A query returns data AND creates a subscription to receive
  * DocActions that affect this data. The querySubId field identifies this subscription, and must
@@ -113,7 +130,7 @@ export interface ActiveDocAPI {
    * docActions that affect this query's results. The subscription remains functional even when
    * tables or columns get renamed.
    */
-  useQuerySet(query: Query): Promise<QueryResult>;
+  useQuerySet(query: ServerQuery): Promise<QueryResult>;
 
   /**
    * Removes the subscription to a Query, identified by QueryResult.querySubId, so that the

app/server/lib/ActiveDoc.ts

@@ -22,8 +22,8 @@ import {
   DataSourceTransformed,
   ForkResult,
   ImportResult,
-  Query,
+  QueryResult,
-  QueryResult
+  ServerQuery
 } from 'app/common/ActiveDocAPI';
 import {ApiError} from 'app/common/ApiError';
 import {mapGetOrSet, MapWithTTL} from 'app/common/AsyncCreate';
@@ -612,7 +612,7 @@ export class ActiveDoc extends EventEmitter {
    * @param {Boolean} waitForFormulas: If true, wait for all data to be loaded/calculated.  If false,
    * special "pending" values may be returned.
    */
-  public async fetchQuery(docSession: OptDocSession, query: Query,
+  public async fetchQuery(docSession: OptDocSession, query: ServerQuery,
                           waitForFormulas: boolean = false): Promise<TableDataAction> {
     this._inactivityTimer.ping();     // The doc is in active use; ping it to stay open longer.
 
@@ -693,7 +693,7 @@ export class ActiveDoc extends EventEmitter {
    * Makes a query (documented elsewhere) and subscribes to it, so that the client receives
    * docActions that affect this query's results.
    */
-  public async useQuerySet(docSession: OptDocSession, query: Query): Promise<QueryResult> {
+  public async useQuerySet(docSession: OptDocSession, query: ServerQuery): Promise<QueryResult> {
     this.logInfo(docSession, "useQuerySet(%s, %s)", docSession, query);
     // TODO implement subscribing to the query.
     // - Convert tableId+colIds to TableData/ColData references
@@ -1356,7 +1356,7 @@ export class ActiveDoc extends EventEmitter {
     }
   }
 
-  private async _fetchQueryFromDB(query: Query, onDemand: boolean): Promise<TableDataAction> {
+  private async _fetchQueryFromDB(query: ServerQuery, onDemand: boolean): Promise<TableDataAction> {
     // Expand query to compute formulas (or include placeholders for them).
     const expandedQuery = expandQuery(query, this.docData!, onDemand);
     const marshalled = await this.docStorage.fetchQuery(expandedQuery);
@@ -1372,7 +1372,7 @@ export class ActiveDoc extends EventEmitter {
     return toTableDataAction(query.tableId, table);
   }
 
-  private async _fetchQueryFromDataEngine(query: Query): Promise<TableDataAction> {
+  private async _fetchQueryFromDataEngine(query: ServerQuery): Promise<TableDataAction> {
     return this._pyCall('fetch_table', query.tableId, true, query.filters);
   }
 

app/server/lib/ExpandedQuery.ts

@@ -1,4 +1,4 @@
-import {Query} from 'app/common/ActiveDocAPI';
+import {ServerQuery} from 'app/common/ActiveDocAPI';
 import {ApiError} from 'app/common/ApiError';
 import {DocData} from 'app/common/DocData';
 import {parseFormula} from 'app/common/Formula';
@@ -10,7 +10,7 @@ import {quoteIdent} from 'app/server/lib/SQLiteDB';
  * formulas.  Use of this representation should be limited to within a
  * trusted part of Grist since it assembles SQL strings.
  */
-export interface ExpandedQuery extends Query {
+export interface ExpandedQuery extends ServerQuery {
   // Errors detected for given columns because of formula issues.  We
   // need to make sure the result of the query contains these error
   // objects.  It is awkward to write a sql selection that constructs
@@ -38,7 +38,7 @@ export interface ExpandedQuery extends Query {
  *
  * If onDemandFormulas is set, ignore stored formula columns, and compute them using SQL.
  */
-export function expandQuery(iquery: Query, docData: DocData, onDemandFormulas: boolean = true): ExpandedQuery {
+export function expandQuery(iquery: ServerQuery, docData: DocData, onDemandFormulas: boolean = true): ExpandedQuery {
   const query: ExpandedQuery = {
     tableId: iquery.tableId,
     filters: iquery.filters,

app/server/lib/GranularAccess.ts

@@ -2,7 +2,7 @@ import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions';
 import { ACLRuleCollection, SPECIAL_RULES_TABLE_ID } from 'app/common/ACLRuleCollection';
 import { ActionGroup } from 'app/common/ActionGroup';
 import { createEmptyActionSummary } from 'app/common/ActionSummary';
-import { Query } from 'app/common/ActiveDocAPI';
+import { ServerQuery } from 'app/common/ActiveDocAPI';
 import { ApiError } from 'app/common/ApiError';
 import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord } from 'app/common/DocActions';
 import { RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions';
@@ -170,7 +170,7 @@ export class GranularAccess implements GranularAccessForBundle {
   public constructor(
     private _docData: DocData,
     private _docClients: DocClients,
-    private _fetchQueryFromDB: (query: Query) => Promise<TableDataAction>,
+    private _fetchQueryFromDB: (query: ServerQuery) => Promise<TableDataAction>,
     private _recoveryMode: boolean,
     private _homeDbManager: HomeDBManager | null,
     private _docId: string) {
@@ -204,13 +204,6 @@ export class GranularAccess implements GranularAccessForBundle {
     this._userAttributesMap = new WeakMap();
   }
 
-  /**
-   * Check whether user can carry out query.
-   */
-  public hasQueryAccess(docSession: OptDocSession, query: Query) {
-    return this.hasTableAccess(docSession, query.tableId);
-  }
-
   public getUser(docSession: OptDocSession): Promise<UserInfo> {
     return this._getUser(docSession);
   }

sandbox/grist/engine.py

@@ -980,13 +980,22 @@ class Engine(object):
     old_tables = self.tables
 
     self.tables = {}
+    sorted_tables = []
     for table_id, user_table in six.iteritems(self.gencode.usercode.__dict__):
-      if isinstance(user_table, table_module.UserTable):
+      if not  isinstance(user_table, table_module.UserTable):
-        self.tables[table_id] = (old_tables.get(table_id) or table_module.Table(table_id, self))
+        continue
+      self.tables[table_id] = table = (
+          old_tables.get(table_id) or table_module.Table(table_id, self)
+      )
+
+      # Process non-summary tables first so that summary tables
+      # can read correct metadata about their source tables
+      key = (hasattr(user_table.Model, '_summarySourceTable'), table_id)
+      sorted_tables.append((key, table, user_table))
+    sorted_tables.sort()
 
     # Now update the table model for each table, and tie it to its UserTable object.
-    for table_id, table in six.iteritems(self.tables):
+    for _, table, user_table in sorted_tables:
-      user_table = getattr(self.gencode.usercode, table_id)
       self._update_table_model(table, user_table)
       user_table._set_table_impl(table)