tableau / connector-plugin-sdk

SDK for Developing Tableau Connector Plugins
https://tableau.github.io/connector-plugin-sdk/
MIT License
105 stars 107 forks source link

Quote column references in expression tests #1194

Closed wnob closed 9 months ago

wnob commented 9 months ago

This ensures that all the expression tests use quotes for column references, even for the calcs columns which tend not to need quotes in most dialects of SQL. Many of the calcs column references were already quoted, but many of them were not. Now, they all are.

Also incremented the patch version in tdvt/version.py and added release notes.

salesforce-cla[bot] commented 9 months ago

Thanks for the contribution! Before we can merge this, we need @wnob to sign the Salesforce Inc. Contributor License Agreement.

lukewrites commented 9 months ago

Thanks, Will, I'll take a look this sprint.

lukewrites commented 9 months ago

Thanks for the PR @wnob, everything looked good on our internal run - :shipit: If you can do the salesforce-cla workflow above that'd be great, but "I couldn't because it's broken" is also a perfectly acceptable alternative.

Curious - did you run into issues with any specific datasources/SQL dialects that prompted the PR? If you did, I'd love to look at your logs to see what happened on the Tableau side.

Thanks again, greatly appreciated

wnob commented 9 months ago

Not sure if it worked properly but this is what it shows when I try to re-sign:

image

I'm working on an integration between Looker and Tableau that has some idiosyncratic column name formatting. We've been collaborating with some of you folks including Thomas Nhan et al.

The error seemed relatively straightforward in this case. We had to add a custom logical query format to accommodate our column name format, but that config seems to apply to the "logical" tests only; not the "expression" tests. Adding quotes to all the expression tests makes it easier for us to sort-of hack it into working with a single idempotent sed command, and I figured it couldn't hurt to try to get that upstreamed, since many of the expression tests used quotes already.