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

fix: update wrong group by definition in tutorial step 12 #1312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MauroLuzzatto
Copy link

@MauroLuzzatto MauroLuzzatto commented Jul 1, 2024

Issue

There seem to have been an issue in the group by definition of step 12 in the tutorial.
It was defined as is_large, whereas the correct definition would have been transaction__is_large.

Fix

mf query --metrics transactions,transaction_usd_na --group-by metric_time,transaction__is_large --order metric_time --start-time 2022-03-20 --end-time 2022-04-01

Error message

(metricflow_env) ➜  folder git:(branch) ✗ mf query --metrics transactions,transaction_usd_na --group-by metric_time, is_large --order metric_time --start-time 2022-03-20 --end-time 2022-04-01
⠹ Initiating query…
ERROR: Got errors while resolving the query.

Error #1:
  Message:

    The given input does not match any of the available group-by-items for
    Measure('transactions'). Common issues are:

      * Incorrect names.
      * No valid join paths exist from the measure to the group-by-item.
        (fan-out join support is pending).
      * There are multiple matching join paths.
        (disambiguation support is pending).

    Suggestions:
      [
        'transaction__quick_buy_transaction',
        'transaction',
        'transaction__ds__day',
        'transaction__customer',
        'transaction__id_order',
        'transaction__is_large',
      ]

  Query Input:

    is_large

  Issue Location:

    [Resolve Query(['transactions', 'transaction_usd_na'])]
      -> [Resolve Metric('transactions')]
        -> [Resolve Measure('transactions')]

Error #1:
  Message:

    The given input does not match any of the available group-by-items for
    Measure('transaction_amount_usd'). Common issues are:

      * Incorrect names.
      * No valid join paths exist from the measure to the group-by-item.
        (fan-out join support is pending).
      * There are multiple matching join paths.
        (disambiguation support is pending).

    Suggestions:
      [
        'transaction__quick_buy_transaction',
        'transaction',
        'transaction__ds__day',
        'transaction__customer',
        'transaction__id_order',
        'transaction__is_large',
      ]

  Query Input:

    quick_buy_transaction

  Issue Location:

    [Resolve Query(['transactions', 'transaction_usd_na'])]
      -> [Resolve Metric('transaction_usd_na')]
        -> [Resolve Measure('transaction_amount_usd')]

Copy link

cla-bot bot commented Jul 1, 2024

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @MauroLuzzatto

@MauroLuzzatto MauroLuzzatto marked this pull request as draft July 1, 2024 11:47
@MauroLuzzatto MauroLuzzatto marked this pull request as ready for review July 1, 2024 11:52
@MauroLuzzatto MauroLuzzatto changed the title fix: update wrong dimension definition in tutorial step 12 fix: update wrong group by definition in tutorial step 12 Jul 1, 2024
@MauroLuzzatto
Copy link
Author

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @MauroLuzzatto

I filled it in twice, bit it doesn't seem to be picked up

@tlento
Copy link
Contributor

tlento commented Jul 24, 2024

Hi @MauroLuzzatto thanks for the fix, and sorry for the delay, I didn't see this somehow.

I don't have the permissions to look into why you aren't passing the CLA checks. I'll follow up internally but in the meantime I guess filling in the form again couldn't hurt. I'm pretty sure the GitHub user name has to exactly match the name on the commit (which is you), so maybe there's some capitalization issue or something?

Would you mind adding a changelog entry to this PR while I try to sort out the CLA issue? That way you'll be sure to get credit when we generate our release notes.

@tlento
Copy link
Contributor

tlento commented Jul 24, 2024

@cla-bot check

Copy link

cla-bot bot commented Jul 24, 2024

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla:yes label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants