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

feat(query): enable min/max pushdown #20994

Merged
merged 3 commits into from
Apr 20, 2021
Merged

feat(query): enable min/max pushdown #20994

merged 3 commits into from
Apr 20, 2021

Conversation

fchikwekwe
Copy link
Contributor

@fchikwekwe fchikwekwe commented Mar 18, 2021

Closes #

@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch 3 times, most recently from 2141b37 to 43f72dc Compare March 18, 2021 10:13
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch 2 times, most recently from 86d9780 to 6d5235f Compare April 1, 2021 20:51
@fchikwekwe fchikwekwe changed the title Feat/min max pushdown feat(query): enable min/max pushdown Apr 1, 2021
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch 3 times, most recently from fe9745e to 89cf604 Compare April 1, 2021 20:59
@fchikwekwe fchikwekwe requested review from a team and wolffcm and removed request for a team April 1, 2021 21:00
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch from 89cf604 to 65c3851 Compare April 2, 2021 03:07
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

I had some questions. I remember that we had a bug in Cloud with group and selectors, so I want to be sure that we have coverage for that particular test case.

"PushDownRangeRule": 1,
"PushDownFilterRule": 1,
])
group_min_test.group_min_bare()
Copy link

Choose a reason for hiding this comment

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

I'm looking at the test that this test extends, and I see that it's filtering by _value:
https://github.com/influxdata/flux/blob/1f16d09633be04ace0b806bbaa90db59ee4ff15a/stdlib/planner/group_min_test.flux#L25

The test for max is similar. This doesn't seem like it would actually validate that these functions work correctly. Am I missing something?

t.appendBounds(colReader)
return true
}

type {{.name}}AggregateMethod func([]int64, []{{.Type}}) (int64, {{.Type}})
type {{.Name}}AggregateAccumulator interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wolffcm This file has a lot of the bug fix work that you were looking for.

@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch from a5383df to 8a4b490 Compare April 6, 2021 22:16
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch 9 times, most recently from 72e7186 to fb21461 Compare April 20, 2021 15:40
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch from 94919fb to d76cd24 Compare April 20, 2021 18:59
@fchikwekwe fchikwekwe requested a review from wolffcm April 20, 2021 19:06
@fchikwekwe fchikwekwe force-pushed the feat/min-max-pushdown branch from d76cd24 to fb29389 Compare April 20, 2021 19:28
@fchikwekwe
Copy link
Contributor Author

Yay! At last :)

@fchikwekwe fchikwekwe merged commit 7bde341 into master Apr 20, 2021
@fchikwekwe fchikwekwe deleted the feat/min-max-pushdown branch April 20, 2021 19:56
dgnorton pushed a commit that referenced this pull request Apr 20, 2021
* feat(query): enable min/max pushdown

* fix(query): fix the group last pushdown to use descending cursors

* test(storage): add read group test with no agg

Co-authored-by: Jonathan A. Sternberg <jonathan@influxdata.com>
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