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(tiger): Implement tupleElement function in snuba #2687

Merged
merged 5 commits into from
May 12, 2022

Conversation

volokluev
Copy link
Member

Context

The new clickhouse sometimes fails to parse queries that have a tupleElement function.

Example:

 SELECT
 (
    toStartOfHour(finish_ts, 'Universal') AS _snuba_time
  ),
  countIf(
    lessOrEquals(
      multiIf(
        equals(tupleElement(tuple('duration', 300), 1), 'lcp'),
        (
          if(
            has(measurements.key, 'lcp'),
            arrayElement(
              measurements.value,
              indexOf(measurements.key, 'lcp')
            ),
            NULL
          ) AS ` _snuba_measurements [ lcp ] `
        ),
        (duration AS _snuba_duration)
      ),
      tupleElement(tuple('duration', 300), 2)
    )
  )
FROM
  transactions_dist PREWHERE equals((transaction_name AS _snuba_transaction), '/')
WHERE
  equals(('transaction' AS _snuba_type), 'transaction')
  AND greaterOrEquals(
    (finish_ts AS _snuba_finish_ts),
    toDateTime('2022-04-11T20:52:46', 'Universal')
  )
  AND less(
    _snuba_finish_ts,
    toDateTime('2022-04-25T20:52:46', 'Universal')
  )
  AND in((project_id AS _snuba_project_id), [ 5853067 ])
GROUP BY
  _snuba_time
ORDER BY
  _snuba_time ASC
LIMIT
  10000 OFFSET 0

This fails with:

 Not found column countIf(lessOrEquals(multiIf(equals(tupleElement(tuple('duration', 300), 1), 'lcp'), 
 if(has(measurements.key, 'lcp'),
  arrayElement(measurements.value, indexOf(measurements.key, 'lcp')), NULL), duration), 
  tupleElement(tuple('duration', 300), 2))) in block. 

Resolving the tupleElement fixes the issue:

SELECT
(
  toStartOfHour(finish_ts, 'Universal') AS _snuba_time
),
countIf(
  lessOrEquals(
    multiIf(
      equals('duration', 'lcp'),
      (
        if(
          has(measurements.key, 'lcp'),
          arrayElement(
            measurements.value,
            indexOf(measurements.key, 'lcp')
          ),
          NULL
        ) AS ` _snuba_measurements [ lcp ] `
      ),
      (duration AS _snuba_duration)
    ),
    300
  )
)
FROM
transactions_dist PREWHERE equals((transaction_name AS _snuba_transaction), '/')
WHERE
equals(('transaction' AS _snuba_type), 'transaction')
AND greaterOrEquals(
  (finish_ts AS _snuba_finish_ts),
  toDateTime('2022-04-11T20:52:46', 'Universal')
)
AND less(
  _snuba_finish_ts,
  toDateTime('2022-04-25T20:52:46', 'Universal')
)
AND in((project_id AS _snuba_project_id), [ 5853067 ])
GROUP BY
_snuba_time
ORDER BY
_snuba_time ASC
LIMIT
10000 OFFSET 0

@volokluev volokluev requested a review from a team as a code owner May 10, 2022 23:59
@volokluev volokluev requested a review from nikhars May 10, 2022 23:59
Copy link
Member

@nikhars nikhars left a comment

Choose a reason for hiding this comment

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

Could you add an end-to-end test that has both tuple aliasing and tuple elements to verify that having both does not break things?

snuba/datasets/storages/errors_v2.py Outdated Show resolved Hide resolved
Comment on lines 59 to +60
TupleUnaliaser(),
TupleElementer(),
Copy link
Member

Choose a reason for hiding this comment

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

Would we always need to guarantee this ordering? Or it does not matter? If we need to guarantee this ordering then please add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Order does not matter

)


TEST_QUERIES = [
Copy link
Member

Choose a reason for hiding this comment

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

Should we add test cases for curried_functions and lambda's as well since this visitor has implementations for them?

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #2687 (d976751) into master (5c893fb) will decrease coverage by 0.07%.
The diff coverage is 87.11%.

❗ Current head d976751 differs from pull request most recent head d9fb0bb. Consider uploading reports for the commit d9fb0bb to get more accurate results

@@            Coverage Diff             @@
##           master    #2687      +/-   ##
==========================================
- Coverage   92.86%   92.78%   -0.08%     
==========================================
  Files         606      612       +6     
  Lines       28537    28995     +458     
==========================================
+ Hits        26500    26903     +403     
- Misses       2037     2092      +55     
Impacted Files Coverage Δ
snuba/cli/devserver.py 14.63% <ø> (ø)
snuba/cli/multistorage_consumer.py 40.20% <0.00%> (-0.42%) ⬇️
snuba/datasets/storages/transactions.py 100.00% <ø> (ø)
snuba/query/functions.py 100.00% <ø> (ø)
snuba/state/__init__.py 71.15% <0.00%> (-0.35%) ⬇️
snuba/cli/subscriptions_executor.py 50.87% <16.66%> (-2.97%) ⬇️
snuba/cli/subscriptions_scheduler_executor.py 55.76% <55.76%> (ø)
snuba/admin/clickhouse/common.py 87.03% <66.66%> (-2.55%) ⬇️
snuba/web/db_query.py 89.56% <66.66%> (+0.04%) ⬆️
snuba/subscriptions/worker.py 95.95% <75.00%> (-1.87%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c893fb...d9fb0bb. Read the comment docs.

@volokluev volokluev requested a review from nikhars May 12, 2022 15:32
@volokluev volokluev merged commit 37cd021 into master May 12, 2022
@volokluev volokluev deleted the volo/tuple_elementer branch May 12, 2022 21:07
@volokluev volokluev restored the volo/tuple_elementer branch June 6, 2022 18:23
volokluev pushed a commit that referenced this pull request Jun 6, 2022
volokluev added a commit that referenced this pull request Jun 6, 2022
@volokluev volokluev deleted the volo/tuple_elementer branch June 13, 2022 19:27
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.

3 participants