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

Support events qualified for multiple conditions. #2801

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

amosbird
Copy link
Collaborator

@amosbird amosbird commented Aug 3, 2018

Currently windowFunnel function only takes the first qualified event condition
into account when operating on one event. This patch extends the
ability.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

if (event_level)
{
this->data(place).add(static_cast<const ColumnVector<UInt32> *>(columns[0])->getData()[row_num], event_level);
this->data(place).add(timestamp, i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, but how to avoid duplicate match?
Eg.
windowFunnel(4)(timestamp, event <= 2, event <=4, event <=6, event <=8)
There's just one row data with event = 2 , but it could have the result 4.

Copy link
Collaborator Author

@amosbird amosbird Aug 3, 2018

Choose a reason for hiding this comment

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

Well, I answered that question in the related comment :-) lemme expand that here with your example.

Consider a table with timestamp : 1 and event : 1

with this windowFunnel(4)(timestamp, event <= 2, event <=4, event <=6, event <=8) invocation, it generates four entries in the event list with the order (1001, 4), (1001, 3), (1001, 2), (1001, 1) and this order is kept via stable sort. (Note, merging sort itself is stable too). Thus, when doing forward scan over the event list, no duplication result would be generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it, nice solution.

@amosbird amosbird force-pushed the master branch 2 times, most recently from 074e4f7 to 972b18b Compare August 4, 2018 05:47
@amosbird amosbird force-pushed the master branch 2 times, most recently from 665b375 to 853e465 Compare August 23, 2018 10:28
Currently windowFunnel function only take the first qualified condition
into account when operating on one event. This patch extends the
ability.
@VadimPlh
Copy link
Contributor

VadimPlh commented Sep 1, 2018

@amosbird Hi! Thanks for your PR! Your code has problems with test(00678_shard_funnel_window). I try to fix it.

@amosbird
Copy link
Collaborator Author

amosbird commented Sep 3, 2018

@VadimPE Hi, I didn't see failures in test 678 and there isn't a related commit in recent commit log. What problem is it?

@VadimPlh
Copy link
Contributor

VadimPlh commented Sep 3, 2018

@amosbird

In this case
CREATE TABLE test.remote_test(uid String, its UInt32, action_code String, day Date) ENGINE = MergeTree(day, (uid, its), 8192);
INSERT INTO test.remote_test SELECT toString(number) AS uid, number % 3 AS its, toString(number % 3) AS action_code, '2000-01-01' FROM system.numbers LIMIT 10000;
SELECT level, COUNT() FROM (SELECT uid, windowFunnel(3600)(toUInt32(its), action_code != '', action_code = '2') AS level FROM remote('127.0.0.{2,3}', test.remote_test) GROUP BY uid) GROUP BY level;

old version: 1 10000
your version:
1 - 6667
2 - 3333

@amosbird
Copy link
Collaborator Author

amosbird commented Sep 3, 2018

Well that's expected. The correct output should be

1 - 6667
2 - 3333

Old version doesn't work when the some filter includes others, and this is what my patch meant to fix.

@VadimPlh
Copy link
Contributor

VadimPlh commented Sep 4, 2018

@amosbird #3032

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