SQL-3343: Rework FLATTEN field-access to avoid repeated work#167
SQL-3343: Rework FLATTEN field-access to avoid repeated work#167jameseh96 wants to merge 2 commits into
Conversation
|
|
mattChiaravalloti
left a comment
mattChiaravalloti
left a comment
There was a problem hiding this comment.
This is a great idea and a useful change. Everything is passing on evergreen so we should be confident this is as correct as the previous solution. I'm marking this as approved but we'll hold off on merging this until after the team triages the associated ticket later on today.
| fn build_flattened_field_access( | ||
| key: Key, | ||
| path: &[String], | ||
| root_schema: &crate::schema::Schema, |
There was a problem hiding this comment.
| root_schema: &crate::schema::Schema, | |
| root_schema: &schema::Schema, |
[nit] We already import crate::schema::self at the top of the file, so we should not need to prefix this with crate:: here.
| let mut expr = mir::Expression::Reference(mir::ReferenceExpr { key }); | ||
| // Avoid cloning the (potentially huge) root schema: narrow from a borrow for the first | ||
| // field, then from the owned narrowed sub-schema for subsequent fields. | ||
| let mut current_schema: Option<crate::schema::Schema> = None; |
There was a problem hiding this comment.
| let mut current_schema: Option<crate::schema::Schema> = None; | |
| let mut current_schema: Option<schema::Schema> = None; |
|
@jameseh96 The team triaged this and decided to move forward with it. I tagged another team member to give it a second look. When you get 2 approvals and address any outstanding comments, we can get this merged. It looks like your commits aren't signed, either. If you are able, can you sign the commits and re-push? Feel free to force push the signed commits to this PR when you sign them. |
jameseh96
force-pushed
the
SQL-3343
branch
2 times, most recently
from
f5f38c3 to
4bbd2f5
Compare
|
Comments addressed, commits signed, re-pushed! |
Previously, algebrize_flattened_field_path was recursive.
For a path a.b.c, the call chain would look like:
algebrize_flattened_field_path(key, [a,b,c])
> construct_field_access_expr( algebrize_flattened_field_path(key,[a,b]), "c" )
> construct_field_access_expr( algebrize_flattened_field_path(key,[a]), "b" )
> construct_field_access_expr( algebrize_flattened_field_path(key,[]), "a" )
> construct_field_access_expr( Reference(key), "a" )
Each construct_field_access_expr call evaluated
```
FieldAccess{...}.schema(state)
```
to decide nullability.
FieldAccess::schema is itself recursive: it calls self.expr.schema(),
which will call `fa.schema(state)` on its contents.
The base case Expression::Reference does `env.get(key).cloned()`,
cloning the root schema.
So each layer of construct_field_access_expr would eventually clone the
root schema, which could be very large. Further, it would then repeat
work done by a lower call:
1. clone root -> a
2. clone root -> a -> b
3. clone root -> a -> b -> c
This could be quite costly.
Change to an iterative approach, tracking the current schema as the call
descends through the path.
For the sample_analytics FLATTEN benchmark this cuts the project
field-access build from ~11.4s to ~3.3s, -71%.
3 participants
Previously, algebrize_flattened_field_path was recursive.
For a path a.b.c, the call chain would look like:
Each
construct_field_access_exprcall evaluatedto decide nullability.
FieldAccess::schema is itself recursive: it calls self.expr.schema(), which will call
fa.schema(state)on its contents.The base case Expression::Reference does
env.get(key).cloned(), cloning the root schema.So each layer of
construct_field_access_exprwould eventually clone the root schema, which could be very large. Further, it would then repeat work done by a lower call:This could be quite costly.
Change to an iterative approach, tracking the current schema as the call descends through the path.
For the sample_analytics FLATTEN benchmark this cuts the project field-access build from ~11.4s to ~3.3s, -71%.