Fix: Escape col names by manually generating node

[ayjayt]

Periods in column names used to completely break rendering. Now they don't.

Mosaic has 0 proper field/string/name parsing:

// TODO: handle case where identifiers are already quoted?
export function parseIdentifier(id: string) {
  return id.split('.');
}

I didn't want to build a parser, and then we'd have to quote all col names anyway. I pass an already initialized Column type which skips string parsing.

More comments about fixes in commit.

Fixes #95

[ayjayt]

column() contructs an actual column node:

https://github.com/uwdata/mosaic/blob/9dc9d0aa1ec9a18aaee3dc87820071f5f3aff1ff/packages/mosaic/sql/src/functions/column.ts#L15-L23

If column's second argument table is other than undefined, query comes out like SELECT "table"."colname"

[ayjayt]

Another candidate: https://github.com/uwdata/mosaic/blob/9dc9d0aa1ec9a18aaee3dc87820071f5f3aff1ff/packages/mosaic/sql/src/ast/column-ref.ts#L35

[ayjayt]

Frustratingly, mosaic doesn't handle identifier syntax properly if a string (completely ignores quotes).

However, round-tripping from a column ref node like this seems to produce properly escaped column names.

A bit speculative.

[domoritz]

Should we try to fix this in mosaic instead?

[ayjayt]

I might submit a PR there, i just can't decide how i would build the parser. You still have to make a modification to DatatableClient in quak because it doesn't yet properly quote table names (which you can't do right now since mosaic breaks) and then it's like do you rather quote table names and escape quotes as well or just type the column like Im doing here

[ayjayt]

So aside from fixing the SQL layer to parse properly...

Also you have this issue where the SQL layer of mosaic uses proper SQL terms (ref, expr, identifier, etc) but the utility layer uses layman's terms like "column" and so it's not clear if they really mean column there to be a column in a colloquial sense or a field identifier, and it looks closer to undefined behavior upon analysis.

[domoritz]

I maintain mosaic so if you have concrete suggestions for how the behavior is broken or the API is inconsistent, please file an issue over there. PRs are super welcome as well.

[ayjayt]

It seems a bit of work on my end but that's okay

Another user has the issue here: uwdata/mosaic#933

So I posed some clarifying questions there.

I would to see this PR go through because its a good solution IMO @manzt but I am going to expand it a bit this week.

[manzt]

Could you apply linting fix?

diff --git a/lib/clients/DataTable.ts b/lib/clients/DataTable.ts
index e42258ab52..7bc88c89b7 100644
--- a/lib/clients/DataTable.ts
+++ b/lib/clients/DataTable.ts
@@ -58,9 +58,11 @@
        let empty = await options.coordinator.query(
                Query
                        .from(table)
-                       .select(options.columns?.map( columnName => {
-                               return column(columnName, undefined);
-                       }) ?? ["*"])
+                       .select(
+                               options.columns?.map((columnName) => {
+                                       return column(columnName, undefined);
+                               }) ?? ["*"],
+                       )
                        .limit(0),
                { type: "arrow" },
        );

deno fmt

[manzt]

Thanks for the addition. I'm happy to push this through to unblock you, but upstream would be ideal.

[ayjayt]

@manzt I'm going to do another review

Upstream is a bit more work and will require several reviews, and imo using an actual column type > string manipulation anyway