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

Query events by date for properties known to be dates #7608

Merged
merged 41 commits into from
Dec 22, 2021

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Dec 9, 2021

Changes

First step in allowing filtering before and after a given date for the events table. When querying properties we know are dates or timestamps but are stored as strings that ClickHouse should be able to parse as DateTime

Example of querying for #6619

  • Hard codes DateTime knowledge into the UI to get a release-able vertical slice
  • Includes basic UI querying behind a feature flag.
  • allows someone to filter any property as if it were a DateTime and returns an empty result set if it is not
  • only definitely returns results for the events table
  • introduces property types and formats to the API so that we can filter our $time property as a DateTime

date-que

How did you test this code?

by adding and running tests, by running the queries manually

running the UI manually with and without the feature flag, filtering on the events table and on an insight

@pauldambra pauldambra marked this pull request as ready for review December 9, 2021 19:00
@@ -230,6 +230,18 @@ def prop_filter_json_extract(
),
params,
)
elif operator == "is_after":
Copy link
Contributor

@macobo macobo Dec 13, 2021

Choose a reason for hiding this comment

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

Update ee/clickhouse/models/test/test_property.py#TEST_PROPERTIES

@pauldambra pauldambra requested a review from macobo December 13, 2021 15:29
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

I don't see "after" or "before" despite the feature flag being enabled, I suppose this may be because I have never ran the branch from #7500? If that is it, then this branch should be rebased on top of it (though #7500 is a draft, so I'm not sure what's the state of work there)

@pauldambra
Copy link
Member Author

I don't see "after" or "before" despite the feature flag being enabled, I suppose this may be because I have never ran the branch from #7500? If that is it, then this branch should be rebased on top of it (though #7500 is a draft, so I'm not sure what's the state of work there)

Hmm, this doesn't depend on #7500

Current state of the branch should let you query any property that ClickHouse knows how to convert as if it was a DateTime and our $time property

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

This will be a great addition, though as a user it's pretty confusing that there are two "greater/less than" options, the second one being "after/before". I think it would be more intuitive if is_date_after/is_date_before were just special cases of after/before UI-side, instead of entirely separate options (e.g. for properties recognized as containing dates/times, you would have a calendar icon on the right of the usual text input, which would turn the filter into before/after).

Zrzut ekranu 2021-12-16 o 20 16 24

and prop.property_definition.format is not None
and prop.property_definition.format == UNIX_TIMESTAMP_IN_SECONDS
):
query = f"AND parseDateTimeBestEffortOrNull(substring({property_expr}, 1, 10)) > %({prop_value_param_key})s"
Copy link
Member

Choose a reason for hiding this comment

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

Poking questions from an ignoramus:
What happens if property_expr results in a non-string? What happens if the value has leading/trailing whitespace? What happens if the datetime string has 10+ characters like 2021-04-01 18:30:00?

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if the datetime string has 10+ characters like 2021-04-01 18:30:00

parseDateTimeBestEffortOrNull matches a range of formats or returns null. If someone sends a datetime that ClickHouse can't parse it isn't matched in the results (because null is neither greater than nor less than any date)

What happens if property_expr results in a non-string? What happens if the value has leading/trailing whitespace?

select parseDateTimeBestEffortOrNull('   2021-12-25 00:00:01')
 -- ✅ 2021-12-25 00:00:01

select parseDateTimeBestEffortOrNull('   2021-12-25 00:00:01   ')
 -- ✅ 2021-12-25 00:00:01

select parseDateTimeBestEffortOrNull('   not a date   ')
 -- ✅ null

select toDateTimeOrNull('   2021-12-25 00:00:01')
 -- ❌ null

select toDateTimeOrNull('   2021-12-25 00:00:01   ')
 -- ❌ null

At the point there are enough examples in the code to refactor this. I'll include a call-out to the parseDateTime docs. Since this enough is only in my head right now

posthog/models/property.py Outdated Show resolved Hide resolved
@pauldambra
Copy link
Member Author

though as a user it's pretty confusing that there are two "greater/less than" options, the second one being "after/before". I think it would be more intuitive if is_date_after/is_date_before were just special cases of after/before UI-side,

agreed.

I think the next step is to merge this and follow up with changes to property/event definitions so the UI can be type aware. And do sensible things like this

@pauldambra pauldambra requested a review from Twixes December 21, 2021 17:39
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Let's get this in

@pauldambra
Copy link
Member Author

Storybook is failing outside of our control. See chromaui/chromatic-cli#485

Will merge this as the changes don't impact the story book

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