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.

Sign up for GitHub

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

[optimize] MRE::typ(): can we avoid recalculating types? #30871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[optimize] MRE::typ(): can we avoid recalculating types? #30871

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c29c55
does recalculating types matter?
mgree Dec 19, 2024
0779168
trying with just `found_typ`
mgree Dec 19, 2024
1ea70a4
rewrite tests, just two small differences in keys
mgree Dec 19, 2024
a8094ec
drop stale comment, per @antiguru
mgree Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/adapter/src/coord/peek.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,13 @@ pub fn create_fast_path_plan<T: Timestamp>(
if dataflow_plan.objects_to_build.len() >= 1 && dataflow_plan.objects_to_build[0].id == view_id
{
let mut mir = &*dataflow_plan.objects_to_build[0].plan.as_inner_mut();
if let Some((rows, ..)) = mir.as_const() {
if let Some((rows, found_typ)) = mir.as_const() {
// In the case of a constant, we can return the result now.
return Ok(Some(FastPathPlan::Constant(
rows.clone()
.map(|rows| rows.into_iter().map(|(row, diff)| (row, diff)).collect()),
// For best accuracy, we need to recalculate typ.
mgree marked this conversation as resolved.
Show resolved Hide resolved
mir.typ(),
found_typ.clone(),
)));
} else {
// If there is a TopK that would be completely covered by the finishing, then jump
Expand Down
2 changes: 1 addition & 1 deletion src/environmentd/tests/testdata/http/ws
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ ws-text
ws-text
{"query": "SELECT 1"}
----
{"type":"Notice","payload":{"message":"{\n \"plans\": {\n \"raw\": {\n \"text\": \"Finish output=[#0]\\n Map (1)\\n Constant\\n - ()\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"Map\": {\n \"input\": {\n \"Constant\": {\n \"rows\": [\n {\n \"data\": []\n }\n ],\n \"typ\": {\n \"column_types\": [],\n \"keys\": []\n }\n }\n },\n \"scalars\": [\n {\n \"Literal\": [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ]\n }\n ]\n }\n }\n },\n \"optimized\": {\n \"global\": {\n \"text\": \"t66:\\n Finish output=[#0]\\n ArrangeBy keys=[[#0]]\\n ReadGlobalFromSameDataflow t65\\n\\nt65:\\n Constant\\n - (1)\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"plans\": [\n {\n \"id\": \"t66\",\n \"plan\": {\n \"ArrangeBy\": {\n \"input\": {\n \"Get\": {\n \"id\": {\n \"Global\": {\n \"Transient\": 65\n }\n },\n \"typ\": {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": [\n []\n ]\n },\n \"access_strategy\": \"SameDataflow\"\n }\n },\n \"keys\": [\n [\n {\n \"Column\": 0\n }\n ]\n ]\n }\n }\n },\n {\n \"id\": \"t65\",\n \"plan\": {\n \"Constant\": {\n \"rows\": {\n \"Ok\": [\n [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n 1\n ]\n ]\n },\n \"typ\": {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": []\n }\n }\n }\n }\n ],\n \"sources\": []\n }\n },\n \"fast_path\": {\n \"text\": \"Explained Query (fast path):\\n Finish output=[#0]\\n Constant\\n - (1)\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"plans\": [\n {\n \"id\": \"Explained Query (fast path)\",\n \"plan\": {\n \"Constant\": [\n {\n \"Ok\": [\n [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n 1\n ]\n ]\n },\n {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": [\n []\n ]\n }\n ]\n }\n }\n ],\n \"sources\": []\n }\n }\n }\n },\n \"insights\": {\n \"imports\": {},\n \"fast_path_clusters\": {},\n \"fast_path_limit\": null,\n \"persist_count\": []\n },\n \"cluster\": {\n \"name\": \"mz_catalog_server\",\n \"id\": {\n \"System\": 2\n }\n },\n \"redacted_sql\": \"SELECT '<REDACTED>'\"\n}","code":"MZ001","severity":"notice"}}
{"type":"Notice","payload":{"message":"{\n \"plans\": {\n \"raw\": {\n \"text\": \"Finish output=[#0]\\n Map (1)\\n Constant\\n - ()\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"Map\": {\n \"input\": {\n \"Constant\": {\n \"rows\": [\n {\n \"data\": []\n }\n ],\n \"typ\": {\n \"column_types\": [],\n \"keys\": []\n }\n }\n },\n \"scalars\": [\n {\n \"Literal\": [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ]\n }\n ]\n }\n }\n },\n \"optimized\": {\n \"global\": {\n \"text\": \"t66:\\n Finish output=[#0]\\n ArrangeBy keys=[[#0]]\\n ReadGlobalFromSameDataflow t65\\n\\nt65:\\n Constant\\n - (1)\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"plans\": [\n {\n \"id\": \"t66\",\n \"plan\": {\n \"ArrangeBy\": {\n \"input\": {\n \"Get\": {\n \"id\": {\n \"Global\": {\n \"Transient\": 65\n }\n },\n \"typ\": {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": [\n []\n ]\n },\n \"access_strategy\": \"SameDataflow\"\n }\n },\n \"keys\": [\n [\n {\n \"Column\": 0\n }\n ]\n ]\n }\n }\n },\n {\n \"id\": \"t65\",\n \"plan\": {\n \"Constant\": {\n \"rows\": {\n \"Ok\": [\n [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n 1\n ]\n ]\n },\n \"typ\": {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": []\n }\n }\n }\n }\n ],\n \"sources\": []\n }\n },\n \"fast_path\": {\n \"text\": \"Explained Query (fast path):\\n Finish output=[#0]\\n Constant\\n - (1)\\n\\nTarget cluster: mz_catalog_server\\n\",\n \"json\": {\n \"plans\": [\n {\n \"id\": \"Explained Query (fast path)\",\n \"plan\": {\n \"Constant\": [\n {\n \"Ok\": [\n [\n {\n \"data\": [\n 45,\n 1\n ]\n },\n 1\n ]\n ]\n },\n {\n \"column_types\": [\n {\n \"scalar_type\": \"Int32\",\n \"nullable\": false\n }\n ],\n \"keys\": []\n }\n ]\n }\n }\n ],\n \"sources\": []\n }\n }\n }\n },\n \"insights\": {\n \"imports\": {},\n \"fast_path_clusters\": {},\n \"fast_path_limit\": null,\n \"persist_count\": []\n },\n \"cluster\": {\n \"name\": \"mz_catalog_server\",\n \"id\": {\n \"System\": 2\n }\n },\n \"redacted_sql\": \"SELECT '<REDACTED>'\"\n}","code":"MZ001","severity":"notice"}}
{"type":"CommandStarting","payload":{"has_rows":true,"is_streaming":false}}
{"type":"Rows","payload":{"columns":[{"name":"?column?","type_oid":23,"type_len":4,"type_mod":-1}]}}
{"type":"Row","payload":["1"]}
Expand Down
4 changes: 1 addition & 3 deletions test/sqllogictest/explain/plan_insights.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,9 +1248,7 @@ EXPLAIN PLAN INSIGHTS AS JSON FOR SELECT 'abc'
"nullable": false
}
],
"keys": [
[]
]
"keys": []
Copy link
Contributor

Choose a reason for hiding this comment

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

It's surprising that the key was wrong in the MirRelationExpr::Constant from which as_const got the type. We might want to investigate this (maybe independently from this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, could you rebase on top of #30881?
I have a theory on what might have happened: take_safely used to not set the best possible key, because it ran the key derivation on the original thing, where the key derivation often has no chance to realize that it's an empty relation (because the caller of take_safely might have determined this in complex ways). If this doesn't change after the rebase, then there are other places where we don't accurately set the keys on constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #30894.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebase is green... which means that we're still getting the less precise types on constants, right? 🤔

}
]
}
Expand Down
Loading