8000 [DYOD] Support window function queries by niklasmohrin · Pull Request #2597 · hyrise/hyrise · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[DYOD] Support window function queries #2597 8000

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

Closed
wants to merge 121 commits into from

Conversation

niklasmohrin
Copy link
Contributor
@niklasmohrin niklasmohrin commented Jul 26, 2023

We would recommend starting a review with the header files and then to go top-down, that is, start at WindowFunctionEvaluator::_on_execute and stepping into the used functions from there. The implementation of SegmentTree is independent from the rest of the code, so should be okay to review in parallel. Similarly, the construction of the operator (lqp translation) is also rather boilerplate-y, so you can do it at any time.

Todos (not necessarily done before intermediate review):

  • check for nullptr chunks
  • investigate slow sorting for query 51
  • benchmark against another implementation
  • implement and benchmark different fan-outs / -ins for partition_and_sort
  • increase test coverage / cover everything except unreachable code
  • better operator description in query plans
  • ...

niklasmohrin and others added 30 commits July 26, 2023 16:24
Upstream said that we should create new `Chunk`s that re-use the
previous segments (they are stored as shared pointers).
Assigning does not work, because `operator[]` returns `AllTypeVariant`,
not a reference.
It appears that someone is renaming our column after the operator has
executed.
This is needed to prevent creating a chunk that has both
`ReferenceSegment`s and `ValueSegments`s, as our output column is always
represented by `ValueSegment`s.
We now reuse all segments from the input table.
If the input table is a reference table, we create a new table with our output value segments and then add
reference segments to this new table in our output table.
This adds `_templated_on_execute` so that the input column type and
window function are compile time constants the whole time. This way, we
don't get template instantiation errors down the line when we resolve
types again.
niklasmohrin and others added 13 commits July 27, 2023 14:04
… the `window_function_evaluator` namespace
We want to use features that gcc 9 doesn't support. Commands to
reproduce:

```sh
sed -i 's/gcc-9/gcc-10/g' Jenkinsfile Dockerfile install_dependencies.sh
sed -i 's/g++-9/g++-10/g' Jenkinsfile Dockerfile install_dependencies.sh
sed -i 's/gcc9/gcc10/g' Jenkinsfile Dockerfile install_dependencies.sh
sed -i 's/g++9/g++10/g' Jenkinsfile Dockerfile install_dependencies.sh
```
@niklasmohrin niklasmohrin marked this pull request as ready for review August 2, 2023 17:23
@ClFeSc ClFeSc requested review from ClFeSc, 23mafi and phkeese August 4, 2023 13:13
Copy link
Contributor
@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

Your code looks very good, although we did find it somewhat hard to read due to a lack of comments and its quite complex logic. Please see our comments, and feel free to ask questions if something is unclear.

With regards, Team 2, @ClFeSc @23mafi @phkeese

@niklasmohrin
Copy link
Contributor Author

As discussed with the supervisors, we will close this PR and open another one later for the final submission. This way, it is easier to browse the code at the point of review.

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.

6 participants
0