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

Allow ScalarExpr in Join keys #2235

Merged
merged 13 commits into from Mar 27, 2020

Conversation

@frankmcsherry
Copy link
Member

frankmcsherry commented Mar 6, 2020

This is the beginning of a PR that means to allow general ScalarExprs in join keys, rather than just columns. There are several cases where this helps, most prominently when there is a single constant value to be joined with other values (e.g. a filter to implement as an indexed look-up). This shows up in @wangandi's work, but also in set intersection work, plausibly.

Anyhow, it also just seems like the right thing to do, and simplifies logic in many cases. In particular, it may obviate the need for simplify.rs which currently exists only to work around this limitation.

@frankmcsherry frankmcsherry force-pushed the frankmcsherry:join_scalarexpr branch from 935c936 to f6c82e7 Mar 20, 2020
@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 20, 2020

Many SLT explain tests are now corrected, but there are a few that are regressions. It looks like it may be due to our "reason about keys after a join" logic, but investigation is underway.

@frankmcsherry frankmcsherry requested a review from wangandi Mar 20, 2020
@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 20, 2020

This is surely not ready to merge yet (no tests, for example) but it does seem to be in a stable state. I've flagged it for review as I suspect that the body of things is starting to shape up, but feel free to await some tests before looking too hard.

test/sqllogictest/tpch.slt Outdated Show resolved Hide resolved
@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 25, 2020

F!@#$!#$!@#$ rebasing. I swear...

@frankmcsherry frankmcsherry force-pushed the frankmcsherry:join_scalarexpr branch from 181d4c0 to 93b440a Mar 25, 2020
@frankmcsherry frankmcsherry force-pushed the frankmcsherry:join_scalarexpr branch from 93b440a to beb3900 Mar 25, 2020
@frankmcsherry frankmcsherry changed the title WIP: Allow ScalarExpr in Join keys Allow ScalarExpr in Join keys Mar 26, 2020
@netlify

This comment has been minimized.

Copy link

netlify bot commented Mar 26, 2020

Deploy request for materializeinc pending review.

Review with commit 0b904a5

https://app.netlify.com/sites/materializeinc/deploys

----
1 l2 l3

# Test that join plans with scalars work in subqueries

This comment has been minimized.

Copy link
@wangandi

wangandi Mar 27, 2020

Member

Just add an explain plan test for this.

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Mar 27, 2020

Author Member

The plan will 100% be something we don't want to maintain, is why I didn't include it.

This comment has been minimized.

Copy link
@wangandi

wangandi Mar 27, 2020

Member

I'm guessing we want to improve the query planning significantly for this one? Alternatively, we can add a "TODO: explain plan for this when better planning around subqueries comes about"

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Mar 27, 2020

Author Member

I have a note that it is totally fine to change the plan. I don't think this particular one is the right place to track "subqueries are badly planned". I think the TODO isn't necessary in that the plan should break if better planning comes around. Lmk if that doesn't sound right to you!

This comment has been minimized.

Copy link
@wangandi
@frankmcsherry frankmcsherry merged commit b1e65f3 into MaterializeInc:master Mar 27, 2020
16 checks passed
16 checks passed
buildkite/tests Build #6223 passed (20 minutes, 7 seconds)
Details
buildkite/tests/bath-lint-and-rustfmt Passed (21 seconds)
Details
buildkite/tests/book-catalog-compatibility-check Passed (38 seconds)
Details
buildkite/tests/bulb-bulb-full-sql-logic-tests Passed (0 seconds)
Details
buildkite/tests/bulb-short-sql-logic-tests Passed (1 minute, 20 seconds)
Details
buildkite/tests/cargo-test Passed (49 seconds)
Details
buildkite/tests/chbench-sanity-check Passed (1 minute, 58 seconds)
Details
buildkite/tests/docker-build Passed (13 minutes, 54 seconds)
Details
buildkite/tests/face-with-monocle-miri-test Passed (56 seconds)
Details
buildkite/tests/metabase-demo Passed (57 seconds)
Details
buildkite/tests/paperclip-clippy-and-doctests Passed (2 minutes, 11 seconds)
Details
buildkite/tests/pipeline Passed (15 seconds)
Details
buildkite/tests/racing-car-testdrive Passed (3 minutes, 38 seconds)
Details
buildkite/tests/shower-streaming-demo Passed (27 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
netlify/materializeinc/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.