feat(snowflake-driver): add ignore case statements flag #8899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

mike-taylor-nomihealth wants to merge 3 commits into cube-js:master from artemishealth:fix-case-statement

Contributor

mike-taylor-nomihealth commented Nov 1, 2024 •

edited by KSDaemon

Loading

Check List

Tests has been run in packages where changes made if available
Linter has been run for changed code
Tests for the changes have been added if not covered yet
Docs have been added / updated if required

Closes #7440
Supersedes #8765

mike-taylor-nomihealth requested a review from a team as a code owner

November 1, 2024 17:40

vercel bot commented Nov 1, 2024 •

edited

Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

8 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
examples-angular-dashboard	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-d3	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-dashboard	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-data-table	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-highcharts	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-material-ui	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-react-pivot-table	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm
examples-vue-query-builder	⬜️ Ignored (Inspect)	Visit Preview		Nov 1, 2024 5:44pm

github-actions bot added the pr:community label

mike-taylor-nomihealth force-pushed the fix-case-statement branch from 71040e4 to 6d01486 Compare

November 1, 2024 17:43

Member

paveltiunov commented Nov 3, 2024

@mike-taylor-nomihealth Thanks for contributing! Could we please put it under the flag enabled by default? Thanks!

paveltiunov self-assigned this

igorlukanin assigned KSDaemon

mike-taylor-nomihealth requested a review from a team as a code owner

December 11, 2024 18:22

vercel bot had a problem deploying to Preview

December 11, 2024 18:23

Failure

Contributor Author

mike-taylor-nomihealth commented Dec 11, 2024 •

edited

Loading

@mike-taylor-nomihealth Thanks for contributing! Could we please put it under the flag enabled by default? Thanks!

I believe I got this all set up properly - let me know if this is not what you meant

Also - just realized #8665 was the same fix, so I implemented it in the suggested format from that bug ticket.

mike-taylor-nomihealth force-pushed the fix-case-statement branch from 59837c2 to c80d27a Compare

December 11, 2024 18:53

vercel bot had a problem deploying to Preview

December 11, 2024 18:53

Failure

mike-taylor-nomihealth force-pushed the fix-case-statement branch 2 times, most recently from 2c834c5 to 2c5054b Compare

December 11, 2024 18:56

vercel bot had a problem deploying to Preview

December 11, 2024 18:56

Failure

mike-taylor-nomihealth force-pushed the fix-case-statement branch from 2c5054b to d2e2dd6 Compare

December 11, 2024 18:56

vercel bot had a problem deploying to Preview

December 11, 2024 18:58

Failure


          Fixing case statements in snowflake driver

82703ec

mike-taylor-nomihealth force-pushed the fix-case-statement branch from d2e2dd6 to c6e56e6 Compare

December 11, 2024 20:23

vercel bot had a problem deploying to Preview

December 11, 2024 20:24

Failure


          Fixes case sensitivity for snowflake to be default and env driven

d0bcf15

Adds env var for the snowflake driver to enable or disable the
case sensitivity and if not set will default to case insensitive

mike-taylor-nomihealth force-pushed the fix-case-statement branch from c6e56e6 to d0bcf15 Compare

December 11, 2024 21:05

vercel bot had a problem deploying to Preview

December 11, 2024 21:06

Failure

KSDaemon requested changes

View reviewed changes

Member

KSDaemon left a comment

Hi @mike-taylor-nomihealth!
Thank you for taking this up! Left some comments! WDYT?

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts Outdated

    
                    await this.execute(connection, 'ALTER SESSION SET TIMEZONE = \'UTC\'', [], false);

                    await this.execute(connection, `ALTER SESSION SET STATEMENT_TIMEOUT_IN_SECONDS = ${this.config.executionTimeout}`, [], false);

                    if (this.ignoreCase) {

Member

KSDaemon Dec 13, 2024

I realized that we mess the value of our env CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_IGNORE_CASE / this.ignoreCase and the value we pass to the Snowflake.

I mean — if we have true — we pass false and vice versa.
Maybe it would be better to align them. Possible options imho are:

Rename our var to smth like CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_RESPECT_CASE. Then having it set to true — means exactly what we set by this statement — not ignoring the case.
Or.... Use CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_IGNORE_CASE as is: if it is true — then — we should pass true to Snowflake. In this case, don't forget to update the default value if var is not set, as we want respect case by default.

Contributor Author

mike-taylor-nomihealth Jan 7, 2025 •

edited

Loading

Thought about this and even though it makes it a little bit janky to keep the CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_IGNORE_CASE that matches the values passed so it matches the env var with the key/value that we're passing in the session - I think that's more straightforward even though it does mean we have the default behavior always hitting that logic path for a !this.ignoreCase

Contributor Author

mike-taylor-nomihealth Jan 7, 2025

Are we absolutely we sure we want to respect case by default for the snowflake driver given that they made the change to ignore it and upper() everthing? It seems non-intuitive that their driver doesn't default to case insensitive when that's the behavior of their UI and everything in between, so if we set the default behavior to be case sensitive it feels like there are a lot of folks using this driver that are going to have to set a value explicitly when the default behavior probably should reflect how snowflake is using it (even if they didn't update their driver settings to reflect that by default.).

I made the change in the PR but am just playing devils advocate here, because it seems like we got here because this ISN'T the default behavior but probably should be because of how SNOWFLAKE decided to update their behaviors.

Member

KSDaemon Jan 7, 2025

Well, I agree with you in general. The idea is first to put this behind the flag and later enable it by default.

KSDaemon changed the title ~~Fixing case statements in snowflake driver~~ feat(snowflake-driver): add ignore case statements flag

KSDaemon reviewed

View reviewed changes

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts Show resolved Hide resolved

Member

KSDaemon commented Jan 6, 2025

Gentle ping @mike-taylor-nomihealth!


          Updating snowflake driver to respect case by default with added overr…

29bc2fc

…ide.

mike-taylor-nomihealth force-pushed the fix-case-statement branch from 9b72949 to 29bc2fc Compare

January 7, 2025 21:31

KSDaemon reviewed

View reviewed changes

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts

    
                    exportBucket: this.getExportBucket(dataSource),

                    resultPrefetch: 1,

                    executionTimeout: getEnv('dbQueryTimeout', { dataSource }),

                    ignoreCase: getEnv('snowflakeQuotedIdentIgnoreCase', { dataSource }),

Member

KSDaemon Jan 16, 2025

here you set ignoreCase, while SnowflakeDriverOptions type has caseSensitiveIdentifiers?: boolean

KSDaemon requested changes

View reviewed changes

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts

    
                resultPrefetch?: number,

                exportBucket?: SnowflakeDriverExportBucket,

                executionTimeout?: number,

                caseSensitiveIdentifiers?: boolean,

Member

KSDaemon Jan 16, 2025

Suggested change

      
              caseSensitiveIdentifiers?: boolean,
          
              identIgnoreCase?: boolean,

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts

    
                    exportBucket: this.getExportBucket(dataSource),

                    resultPrefetch: 1,

                    executionTimeout: getEnv('dbQueryTimeout', { dataSource }),

                    ignoreCase: getEnv('snowflakeQuotedIdentIgnoreCase', { dataSource }),

Member

KSDaemon Jan 16, 2025

Suggested change

      
                  ignoreCase: getEnv('snowflakeQuotedIdentIgnoreCase', { dataSource }),
          
                  identIgnoreCase: getEnv('snowflakeQuotedIdentIgnoreCase', { dataSource }),

packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts

    
                    // We only want to ignore the case if someone sets the value to false explicitly since the default assumption

                    // is that casing matters

                    if (!this.ignoreCase) {

Member

KSDaemon Jan 16, 2025

Suggested change

      
                  if (!this.ignoreCase) {
          
                  if (!this.identIgnoreCase) {

KSDaemon mentioned this pull request

feat(snowflake-driver): Add ignore case statements flag #9131

Merged

4 tasks

Member

KSDaemon commented Jan 23, 2025

Okay... I took it into my own hands.: #9131.

KSDaemon closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels