Skip to content
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.

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

Fixed a bigquery SQL query for int values when we choose revenue, but… #1463

Merged
merged 3 commits into from Jul 20, 2023

Conversation

SGudbrandsson
Copy link
Contributor

@SGudbrandsson SGudbrandsson commented Jul 14, 2023

… not binomial

When you select a revenue number and chooose not binomial, you get the following error:
image

Features and Changes

This fix adds a column alias so SQL queries end up being correct.

Before the change

    SELECT
      anonymous_id as anonymous_id,
      m.value as value, -- We're looking for a column called value
      CAST(m.timestamp as DATETIME) as timestamp,
      CAST(m.timestamp as DATETIME) as conversion_start,
      CAST(m.timestamp as DATETIME) as conversion_end
    FROM
      (
        SELECT
          user_id,
          user_pseudo_id as anonymous_id,
          TIMESTAMP_MICROS(event_timestamp) as timestamp,
          value_param.value.int_value  -- This is the bug here. This fix is to cast it "as `value`"
        FROM
          `XXXXX`.`XXXX`.`events_intraday_*`,
          UNNEST (event_params) AS value_param
        WHERE
          event_name = 'purchase'
          AND value_param.key = 'value'
          AND _TABLE_SUFFIX BETWEEN '20230714' AND '20230717'
      ) m
    WHERE
      CAST(m.timestamp as DATETIME) >= DATETIME("2023-07-14 13:53:00")
      AND CAST(m.timestamp as DATETIME) <= DATETIME("2023-07-17 15:03:55")

After fix

    SELECT
      anonymous_id as anonymous_id,
      m.value as value,
      CAST(m.timestamp as DATETIME) as timestamp,
      CAST(m.timestamp as DATETIME) as conversion_start,
      CAST(m.timestamp as DATETIME) as conversion_end
    FROM
      (
        SELECT
          user_id,
          user_pseudo_id as anonymous_id,
          TIMESTAMP_MICROS(event_timestamp) as timestamp,
          value_param.value.int_value as value
        FROM
          `XXXX`.`XXXX`.`events_intraday_*`,
          UNNEST (event_params) AS value_param
        WHERE
          event_name = 'purchase'
          AND value_param.key = 'value'
          AND _TABLE_SUFFIX BETWEEN '20230714' AND '20230717'
      ) m
    WHERE
      CAST(m.timestamp as DATETIME) >= DATETIME("2023-07-14 13:53:00")
      AND CAST(m.timestamp as DATETIME) <= DATETIME("2023-07-17 15:03:55")
  • Closes (add link to issue here)

Dependencies

Testing

Screenshots

@@ -77,7 +77,7 @@ WHERE
? ",\n event_value_in_usd as value"
: type === "binomial"
? ""
: `,\n value_param.value.${type === "count" ? "int" : "float"}_value`
: `,\n value_param.value.${type === "count" ? "int" : "float"}_value` as `value`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I think you have a couple extra backticks. It should be the following instead:

Suggested change
: `,\n value_param.value.${type === "count" ? "int" : "float"}_value` as `value`
: `,\n value_param.value.${type === "count" ? "int" : "float"}_value as value`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer, but that's not correct.
In SQL you surround fields when you want to be explicit.
An example is when you have a field that's called "from", you need to surround it with backticks.
Otherwise the field name is taken as a keyword and you'll get an error running the query.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inside a template string in Typescript, which also uses backticks, so the code you have now is a syntax error and will not compile since those internal backticks are not escaped.

Since we're hardcoding the column name to value, backtick escaping is not necessary like it would be if the column name was user-supplied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha.

I just updated it and removed the extra backticks.
Thanks for pointing this out

Fixing extra backticks
@Auz Auz merged commit 89dbe79 into growthbook:main Jul 20, 2023
1 of 2 checks passed
@lukesonnet lukesonnet mentioned this pull request Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants