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.

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

cleanup(inc-723): always run LowCardinalityProcessor, remove ConditionSimpllifier #5820

Merged
merged 2 commits into from
Apr 26, 2024
Merged

cleanup(inc-723): always run LowCardinalityProcessor, remove ConditionSimpllifier #5820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9cdce9
remove ConditionSimplifierProcessor
volokluev Apr 25, 2024
708dc0b
Merge branch 'master' into volo/oncall/run_cardinality_caster_always
volokluev Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion snuba/datasets/configuration/discover/entities/discover_transactions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ query_processors:
- processor: BasicFunctionsProcessor
- processor: ApdexProcessor
- processor: FailureRateProcessor
- processor: ConditionSimplifierProcessor
- processor: LowCardinalityProcessor
args:
columns:
Expand Down
1 change: 0 additions & 1 deletion snuba/datasets/configuration/transactions/entities/transactions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,6 @@ query_processors:
{ processor: BasicFunctionsProcessor },
{ processor: ApdexProcessor },
{ processor: FailureRateProcessor },
{ processor: ConditionSimplifierProcessor },
{
processor: LowCardinalityProcessor,
args:
Expand Down
83 changes: 0 additions & 83 deletions snuba/query/processors/logical/condition_simplifier_processor.py
View file
Open in desktop

This file was deleted.

9 changes: 2 additions & 7 deletions snuba/query/processors/logical/low_cardinality_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from dataclasses import replace

from snuba import settings, state
from snuba import state
from snuba.query.expressions import Column, Expression, FunctionCall, Literal
from snuba.query.logical import Query
from snuba.query.processors.logical import LogicalQueryProcessor
Expand Down Expand Up @@ -41,12 +41,7 @@ def transform_expressions(exp: Expression) -> Expression:
),
)

if (
state.get_int_config(
"use.low.cardinality.processor", settings.USE_CARDINALITY_CASTER
)
== 0
):
if state.get_int_config("use.low.cardinality.processor", 1) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

Enabling this by default caused some test failures last time that blocked sentry master, have we checked it won't happen again?

Copy link
Member Author

Choose a reason for hiding this comment

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

The processor now works differently, the ifNull(col, "") is removed, instead it casts to a Nullable(String). I ran some of the failed tests locally with the new processor and they passed

return

query.transform_expressions(transform_expressions)
1 change: 0 additions & 1 deletion snuba/settings/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,6 @@ class RedisClusters(TypedDict):
# yaml file as well because we validate them. By skipping these steps in production environments
# we save ~2s on startup time
VALIDATE_DATASET_YAMLS_ON_STARTUP = False
USE_CARDINALITY_CASTER = 0

USE_NEW_QUERY_PIPELINE_SAMPLE_RATE = 0.0
TRY_NEW_QUERY_PIPELINE_SAMPLE_RATE = 0.0
Expand Down
1 change: 0 additions & 1 deletion snuba/settings/settings_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,3 @@
VALIDATE_DATASET_YAMLS_ON_STARTUP = True
USE_NEW_QUERY_PIPELINE_SAMPLE_RATE = 1.0
TRY_NEW_QUERY_PIPELINE_SAMPLE_RATE = 1.0
USE_CARDINALITY_CASTER = 1
48 changes: 0 additions & 48 deletions tests/test_snql_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,54 +1391,6 @@ def test_hexint_in_condition(self) -> None:
data = json.loads(response.data)
assert "9533608433997996441" in data["sql"], data["sql"] # Hexint was applied

def test_allocator_clickhouse_bug(self) -> None:
response = self.post(
"/discover/snql",
data=json.dumps(
{
"dataset": "events",
"query": f"""
MATCH (discover)
SELECT
divide(count(), divide(320519.0, 60)) AS `epm`,
quantile(0.5)(duration) AS `p50`,
countIf(notIn(transaction_status, tuple(2, 0, 1))) AS `failure_count`,
transaction BY transaction
WHERE
type = 'transaction'
AND release IN tuple('1123581321345589')
AND environment IN array('prod', 'dev')
AND timestamp >= toDateTime('{self.base_time.isoformat()}')
AND timestamp < toDateTime('{self.next_time.isoformat()}')
AND project_id IN tuple({self.project_id})
HAVING
countIf(notIn(transaction_status, tuple(2, 0, 1))) AS `failure_count` > 0.0
ORDER BY
countIf(notIn(transaction_status, tuple(2, 0, 1))) AS `failure_count` DESC
LIMIT
6 OFFSET 0""",
"legacy": True,
"app_id": "legacy",
"tenant_ids": {
"organization_id": self.org_id,
"referrer": "join.tag.test",
},
"parent_api": "/api/0/issues|groups/{issue_id}/tags/",
}
),
)

assert response.status_code == 200
data = json.loads(response.data)
assert (
"equals((cast(release, 'Nullable(String)') AS _snuba_release), '1123581321345589')"
in data["sql"]
)
assert (
"has(['prod', 'dev'], (cast(environment, 'Nullable(String)') AS _snuba_environment))"
in data["sql"]
)

def test_low_cardinality_processor(self) -> None:
response = self.post(
"/discover/snql",
Expand Down
Loading