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

adapter: remove QGM code #17139

Merged
merged 6 commits into from Jan 16, 2023
Merged

Conversation

aalexandrov
Copy link
Contributor

@aalexandrov aalexandrov commented Jan 13, 2023

!!! EDIT: THERE IS ONE MORE PR REMOVING QGM STUFF: #17371 !!!
If this one is reverted at some point, then that one should also be included!

This also includes #17119

Motivation

  • This PR refactors existing code.

Removes the experimental QGM code, as we currently don't have concrete plans to bring it to a production-ready state.

Tips for reviewer

This also includes the changes in #17119 in order to avoid rebase conflicts (that is, we should merge this after #17119).

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, there is a
    companion cloud PR to account for those changes that is tagged with
    the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • The experimental qgm_optimizations session variable is no longer available and the corresponding optimization stage is removed from the optimizer. For people that have not interfered with that session variable this should not cause any changes. People that have relied on the experimental outer-to-inner join simplification in the code may experience plan regressions, but this is highly unlikely and we have not provided any guarantees about this code.

@aalexandrov aalexandrov added A-optimization Area: query optimization and transformation A-COMPUTE Topics related to the Compute layer labels Jan 13, 2023
@aalexandrov aalexandrov force-pushed the remove_qgm branch 2 times, most recently from b1e5db7 to 74d69e5 Compare January 13, 2023 15:26
@aalexandrov aalexandrov self-assigned this Jan 13, 2023
@aalexandrov aalexandrov requested review from a team as code owners January 16, 2023 12:32
Copy link
Contributor

@philip-stoev philip-stoev left a comment

Choose a reason for hiding this comment

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

Test changes OK.

@teskje teskje self-requested a review January 16, 2023 13:06
Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

Thanks!

Grepping through the code, there are two instances of "qgm" left:

  • The design doc, which is probably fine to keep.
  • A comment in the 101 Query Compilation doc. We might want to remove that too.
    <!--
    # Future Pipeline
    [Diagram](https://docs.google.com/drawings/d/1Fil1-oYy3PkP3bD7WoZphW319Pj60cMAs2HcS9A21uo/edit)
    [Design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/20210707_qgm_sql_high_level_representation.md)
    * [`SQL ⇒ AST`](https://github.com/MaterializeInc/materialize/blob/main/src/sql-parser).
    * Parsing the SQL query
    * `AST ⇒ QGM`.
    * Name resolution.
    * `QGM ⇒ QGM`.
    * Optimizing rewrites + decorrelation + more optimizing rewrites.
    * `QGM ⇒ MIR`.
    * Lowering.
    * [`MIR ⇒ MIR`](https://github.com/MaterializeInc/materialize/blob/main/src/transform).
    * Optimizations. What this looks like is to be determined.
    * Some optimizations may become redundant after optimizing rewrites are added.
    * Note that we may be able to eliminate the per-view/cross-view distinction by modifying MIR to have more than one starting point.
    * `MIR ⇒ LIR`.
    * `LIR ⇒ TDO`.
    -->

Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

Thanks!

@aalexandrov aalexandrov merged commit bd24668 into MaterializeInc:main Jan 16, 2023
@aalexandrov aalexandrov removed request for a team and ggevay January 16, 2023 15:41
@aalexandrov aalexandrov deleted the remove_qgm branch January 16, 2023 15:41
@ggevay ggevay mentioned this pull request Jan 26, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-COMPUTE Topics related to the Compute layer A-optimization Area: query optimization and transformation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants