table names with '.' do not get sanitized when doing a count on y-axis

[Light2Dark]

Hi, thanks for the awesome library! Ran into this issue when building a chart for a table with '.' in the column name.
Or should users sanitize the table / query beforehand? If so, how..🤔

To reproduce:

import * as vg from "@uwdata/vgplot";

vg.coordinator().databaseConnector(vg.wasmConnector());

vg.coordinator().exec(vg.loadCSV("pokemon", "https://gist.githubusercontent.com/armgilles/194bcff35001e7eb53a2a8b441e8b2c6/raw/92200bc0a673d5ce2110aaad4544ed6c4010f687/pokemon.csv"));

const chart = vg.plot(
  vg.rectY(vg.from("pokemon"), { x: "Sp. Def", y: vg.count() })
);

Error:

installHook.js:1 Error: Binder Error: Referenced table "Sp" not found!
Candidate tables: "source"

LINE 1: ..."Sp. Atk", count(*) AS "y" FROM "Pokemon" AS "source" GROUP BY "Sp"." Atk"
                                                                          ^
    at O.onMessage (duckdb-browser-DKjldGPy.js?v=bc8e0c06:12093:12)

[Light2Dark]

Hi, would you like me to make a PR?
These are the fixes I found, basically instead of using the raw string, we use the ColumnRefNode class.
It doesn't go through the logic for using identifiers which splits by .

vgplot/plot/src/marks/Mark.js

 export function markQuery(channels, table, skip = []) {
   const q = Query.from({ source: table });
-  const dims = new Set;
+  const dimToFields = new Map();
   let aggr = false;
 
   for (const c of channels) {
@@ -233,15 +233,15 @@ export function markQuery(channels, table, skip = []) {
       if (isAggregateExpression(field)) {
         aggr = true;
       } else {
-        if (dims.has(as)) continue;
-        dims.add(as);
+        if (dimToFields.has(as)) continue;
+        dimToFields.set(as, field);
       }
       q.select({ [as]: field });
     }
   }
 
   if (aggr) {
-    q.groupby(Array.from(dims));
+    q.groupby(...dimToFields.values());
   }

Tables was also broken

vgplot/inputs/src/Table.js

@@ -226,10 +226,11 @@ export class Table extends Input {
    */
   query(filter = []) {
     const { limit, offset, schema, sortColumn, sortDesc } = this;
-    return Query.from(this.sourceTable())
-      .select(schema.map(s => s.column))
+    const table = this.sourceTable();
+    return Query.from(table)
+      .select(schema.map(s => column(s.column)))
       .where(filter)
-      .orderby(sortColumn ? (sortDesc ? desc(sortColumn) : sortColumn) : [])
+      .orderby(sortColumn ? (sortDesc ? desc(column(sortColumn)) : column(sortColumn)) : [])
       .limit(limit)
       .offset(offset);
   }

[ayjayt]

I'm discussing this issue with a maintainer but for right now my solution was to, instead of passing a string as the column name, construct the column object (like you're doing @Light2Dark ).

I think the code needs to be scanned quite a bit though, the idea being:

The SQL layer should respect SQL rules regarding quotes and dots.

Intermediate layers maybe not if they intend to simplify the API to a simple table-column API, but then will have to properly construct (like you're doing) or escape the field identifiers.

So @domoritz, could it be just simple to say that the SQL layer is the only layer that should be strict SQL syntax and that all other layers should assume a simple table-column layout? (as opposed to subfields, json fields, etc, that SQL understands).

I could make concrete suggestions if I understood the author intention.

[domoritz]

Thanks for looking at this issue. I think yes, only the SQL layer should use SQL semantics and above that it's pull request ably easiest to treat any field names as identifiers not e.g. nested accesses. But I'm not 100% sure whether any feature is relying on the current behavior.

We may still want support for an escape hatch to override how fields are parsed but by default treat inputs from the higher layers as field names.