Skeleton for QGM generation from Hir #8801

asenac · 2021-10-26T13:01:42Z

Motivation

This PR adds a known-desirable feature. MaterializeInc/database-issues#2474

This PR is the first deliverable of the QGM effort (#8073). It contains the model definition as described in the corresponding design doc, plus a module that converts queries from Hir into QGM.

The support for the rest of Hir's operator will be added in separate PRs.

Another followup PRs will contain the rewrite engine for transforming query graph models as well as the module for lowering QGM to Mir.

The code added by this PR is not used by the actual product and it's only compiled in test config, since it is only exercised by unit tests.

This is a subset of #8775.

Tips for reviewer

This first commit contains the definition of the Query Graph Model as described here, the representation that will be used for performing SQL-to-SQL transformations. It also includes code for generating graphviz graphs from query graph models. This generator will be used extensively for debugging and testing.

The second commit contains the skeleton of the module that converts Hir into QGM. It also includes a basic data driven test dumping the resulting graph. TestCatalog needs to be extended so that actual tables and views can be injected into it to test queries referencing actual catalog items.

The third commit adds a README file basically explaining how to render the graphviz graphs that are the output of the tests, for visually validating the.

The forth commit adds support for Hir's Map, Project and Literal. This is a commit that can be used as a reference when extending the Hir->QGM generator.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR adds a release note for any
user-facing behavior changes.

asenac · 2021-10-26T13:08:39Z

The graphs in basic test included in this PR are:

wangandi

Most of my feedback are just requests for more comments in the .rs files.

src/sql/src/query_model/dot_generator.rs

src/sql/src/query_model/mod.rs

src/sql/src/query_model/hir_generator.rs

src/sql/src/query_model/mod.rs

wangandi · 2021-11-01T18:53:38Z

src/sql/src/query_model/mod.rs

+    pub limit: Option<Expr>,
+    pub offset: Option<Expr>,


Can limit and offset ever be anything beside literal uints?

Not sure about the standard, but at least Postgres supports anything that is a constant within the context of the query block, for example, lateral column references.

asenac=> select * from t1, lateral(select * from t2 limit t1.f1 offset t1.f2) foo; f1 | f2 | f1 | f2 ----+----+----+---- (0 rows) asenac=>

Why is translating HirRelationExpr::TopK to QGM currently an open question according to #8073?

Hmm...neither HirRelationExpr nor MirRelationExpr supports limit or offset being anything besides literal uints. Thus, I lean towards having limit and offset be uints until 1) we decide to work on adding support for generalized limit/offset to the public interface or 2) we have a DSL for creating a QGM directly so we can test the generalized limit/offset.

Because TopK is the combination QGM's Grouping and Select operator in a single operator, so I guess the only option we have is to add a TopK box in QGM.

BTW, there is an issue tracking adding support for non-literal limit/offset https://github.com/MaterializeInc/materialize/issues/7071

src/sql/src/query_model/scalar_expr.rs

aalexandrov

Great work, thanks for starting this! I have some remarks but most of them are minor.

My main concern in terms around the (1) precision, (2) encapsulation, and (3) ergonomics the current QGM model encoding.

Regarding (1), the design doc lists quite a few constraints that are currently implicit. I think it will be better if we can model them directly as syntactic restrictions, even at the cost of a bit bloated model code .

Regarding (2), the model currently models vertex (box) and edge (quantifier) identity not through references, but through usize-typed IDs, and requires explicit lookup to follow those. This leaves the door open for dangling references and requires explicit GC calls to free up the model from unused vertices or objects.

Regarding (3), I hope that the need of a dedicated type QueryBox that factors out duplicated attributes can be removed with a lightweight rule-based macro.

I'll try to sketch something minimal along the above lines, but I'm sure that @asenac has already invested exploring similar ideas, so it will be great to have a summary of his experience here as well.

src/sql/src/query_model/README.md

src/sql/src/query_model/test.rs

src/sql/src/query_model/scalar_expr.rs

src/sql/src/query_model/mod.rs

asenac · 2021-11-02T12:51:00Z

Regarding (1), I think there is a trade-off between enforcing constraint through syntax and providing a generic API that can be used to traverse and alter a graph generically. I think we could enforce some of those constraints through a validate method that, in debug mode is invoked before and after each rewrite rule and, in release mode, it gets called at least once, which would still allow us to write generic code independent from the box type.

src/sql/src/query_model/mod.rs

antiguru

LGTM, I left some minor comments.

src/sql/src/query_model/hir_generator.rs

antiguru · 2021-11-04T07:38:22Z

src/sql/src/query_model/mod.rs

+
+/// A Query Graph Model instance represents a SQL query.
+/// See: https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/20210707_qgm_sql_high_level_representation.md
+#[derive(Debug)]


Derive Default.

src/sql/src/query_model/mod.rs

src/sql/tests/querymodel/basic

antiguru · 2021-11-04T07:53:11Z

src/sql/src/query_model/dot_generator.rs

+        for (correlated_q, quantifiers) in correlation_info.iter() {
+            for q in quantifiers.iter() {
+                self.new_line(&format!(
+                    "Q{0} -> Q{1} [ label = \"correlation\", style = filled, color = red ]",


Nit: Use a different color since some folks can't see red as a distinct color.

wangandi

Some minor comments re: panics in the code.

wangandi · 2021-11-05T16:22:54Z

src/sql/src/query_model/hir.rs

+                BoxScalarExpr::Literal(row.clone(), col_type.clone())
+            }
+            HirScalarExpr::Column(c) => {
+                assert!(c.level == 0, "correlated columns not yet supported");


change this to assert_eq!(c.level, 0, "correlated columns ...") so that the level prints out if the equality fails.

This assert is removed by the following patch in #8969, so it won't last very long

wangandi · 2021-11-05T16:56:48Z

src/sql/src/query_model/hir.rs

+            }
+            position -= ib.columns.len();
+        }
+        panic!("column not found")


Can the panics in this file return the column that was not found? Also, should they be errors since eventually QGM will be used for SQL resolution, so it may be necessary to throw an error in the case of a malformed SQL query.

Sure, I'll change this to be an error instead. But this panic is only reached in case of a malformed Hir, which the planner should never produce. Perhaps I should use unreachable! instead.

aalexandrov

LGTM!

main got broken due to merge skew from these two PRs: MaterializeInc#8976 MaterializeInc#8801 Signed-off-by: Petros Angelatos <[email protected]>

asenac requested review from aalexandrov, uce and wangandi October 26, 2021 13:01

aalexandrov added the A-optimization Area: query optimization and transformation label Oct 26, 2021

wangandi reviewed Nov 1, 2021

View reviewed changes

aalexandrov suggested changes Nov 1, 2021

View reviewed changes

asenac force-pushed the qgm-model-hir branch 6 times, most recently from b2c71d4 to 2f3de5a Compare November 2, 2021 12:06

wangandi reviewed Nov 2, 2021

View reviewed changes

src/sql/src/query_model/mod.rs Outdated Show resolved Hide resolved

asenac force-pushed the qgm-model-hir branch 2 times, most recently from 1e069cc to 5d3c6a7 Compare November 3, 2021 14:47

aalexandrov requested review from a team and removed request for a team November 3, 2021 17:51

asenac force-pushed the qgm-model-hir branch from 5d3c6a7 to 3d0b491 Compare November 4, 2021 07:38

antiguru approved these changes Nov 4, 2021

View reviewed changes

asenac force-pushed the qgm-model-hir branch 7 times, most recently from 12c7ae9 to bb24199 Compare November 4, 2021 13:09

ore,expr: move IdGen into ore crate

737c688

asenac force-pushed the qgm-model-hir branch from bb24199 to 7ca8297 Compare November 4, 2021 14:05

asenac force-pushed the qgm-model-hir branch 2 times, most recently from 1c11b61 to ccb5c9a Compare November 4, 2021 16:33

Model and dot generator

badffd0

asenac force-pushed the qgm-model-hir branch from ccb5c9a to da7eec5 Compare November 4, 2021 16:39

asenac added 3 commits November 4, 2021 17:47

Hir -> QGM generator skeleton

e513a15

Add README

f4b2f5b

qgm: Map, Project, Literal support

a8d13a4

asenac force-pushed the qgm-model-hir branch from da7eec5 to a8d13a4 Compare November 4, 2021 16:47

asenac requested review from aalexandrov and wangandi November 4, 2021 16:50

asenac mentioned this pull request Nov 5, 2021

Skeleton for QGM lowering into Mir #8969

Merged

2 tasks

wangandi approved these changes Nov 5, 2021

View reviewed changes

aalexandrov mentioned this pull request Nov 8, 2021

Introduce a SET option to enable/disable QGM per connection #8990

Merged

2 tasks

aalexandrov approved these changes Nov 8, 2021

View reviewed changes

asenac merged commit a6bb9a2 into MaterializeInc:main Nov 8, 2021

petrosagg added a commit to petrosagg/materialize that referenced this pull request Nov 10, 2021

sql: re-add datadriven crate

00f56ec

main got broken due to merge skew from these two PRs: MaterializeInc#8976 MaterializeInc#8801 Signed-off-by: Petros Angelatos <[email protected]>

petrosagg mentioned this pull request Nov 10, 2021

sql: re-add datadriven crate #9024

Merged

Skeleton for QGM generation from Hir #8801

Skeleton for QGM generation from Hir #8801

Uh oh!

Conversation

asenac commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

asenac commented Oct 26, 2021

Uh oh!

wangandi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aalexandrov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asenac commented Nov 2, 2021

Uh oh!

Uh oh!

antiguru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangandi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asenac Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aalexandrov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

asenac commented Oct 26, 2021 •

edited

Loading

asenac Nov 5, 2021 •

edited

Loading