cube icon indicating copy to clipboard operation
cube copied to clipboard

fix(snowflake): fixing the snowflake schema fetcher logic (#7440)

Open christianallred-nomi opened this issue 1 year ago • 7 comments

Check List

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

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

christianallred-nomi avatar Sep 03 '24 18:09 christianallred-nomi

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 Sep 3, 2024 6:35pm
examples-react-d3 ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-react-dashboard ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-react-data-table ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-react-highcharts ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-react-material-ui ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-react-pivot-table ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm
examples-vue-query-builder ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 6:35pm

vercel[bot] avatar Sep 03 '24 18:09 vercel[bot]

Hi @christianallred-nomi đź‘‹

Thanks for your contribution! Do you think this is a breaking change?

igorlukanin avatar Sep 04 '24 11:09 igorlukanin

It should not be no.

A while ago snowflake made a decision to always cap case their responses. This caused an Outage for us. And is what causes the bug this or is aiming to fix with cube.

The fact that this is working for some snowflake users means they have this setting set globally instead of by session.

I liked this solution because it fixes the problem for affected users but should leave the existing users the same.

I have not had time to test this 100% however so if someone with a working snowflake instance can test it, that would be great:

christianallred avatar Sep 06 '24 03:09 christianallred

An alternate decision that would not require a session update would be to update the snowflake driver to overwrite the result parser function

christianallred avatar Sep 06 '24 04:09 christianallred

@christianallred Thanks for contributing it! I think it's definitely great feature however as of now it should go under the flag which we'd enable by default going forward.

paveltiunov avatar Sep 09 '24 15:09 paveltiunov

@christianallred Thanks for contributing it! I think it's definitely great feature however as of now it should go under the flag which we'd enable by default going forward.

Whatever y'all decide is fine. It’s just broken for our use case until a fix is decided on. FWIW I don’t really have skin in the game anymore as I am leaving the org that was looking at cube as a solution. Cheers :)

christianallred avatar Sep 09 '24 16:09 christianallred

@paveltiunov any info about this? I'm having issue with the undefined in snowflake https://github.com/cube-js/cube/issues/7440

mattssll avatar Oct 11 '24 16:10 mattssll

@mattssll We're open for this change if it'd land under the flag. If you have capacity to fix PR we'd be happy to merge it.

paveltiunov avatar Oct 29 '24 05:10 paveltiunov

@christianallred-nomi if you don't know, here are a few tips on how to add flag. It's pretty easy!

Add a new env in env.ts, something like:

  /**
   * Snowflake flag controlling QUOTED_IDENTIFIERS_IGNORE_CASE.
   * Defaults to FALSE if not set
   */
  snowflakeQuotedIdentIgnoreCase: ({
    dataSource
  }: {
    dataSource: string,
  }) => (
    const val = process.env[
      keyByDataSource(
        'CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_IGNORE_CASE',
        dataSource,
      )
    ];
    if (val) {
      if (val.toLocaleLowerCase() === 'true') {
        return true;
      } else if (val.toLowerCase() === 'false') {
        return false;
      } else {
        throw new TypeError(
          `The ${
            keyByDataSource(
              'CUBEJS_DB_SNOWFLAKE_QUOTED_IDENTIFIERS_IGNORE_CASE',
              dataSource,
            )
          } must be either 'true' or 'false'.`
        );
      }
    } else {
      return false;
    }
  ),

Then read and store the flag value in driver's [constructor](https://github.com/cube-js/cube/blob/master/packages/cubejs-snowflake-driver/src/SnowflakeDriver.ts#L253, smth like:

this.ignoreCase = getEnv('snowflakeQuotedIdentIgnoreCase', { dataSource })

And check it in the initConnection(), or simply pass as arg to execute.

if (this.ignoreCase) {
   await this.execute(connection, 'ALTER SESSION SET QUOTED_IDENTIFIERS_IGNORE_CASE = FALSE', [], false);
}

KSDaemon avatar Dec 06 '24 10:12 KSDaemon