Commit 8c7f2a8
authored
[presto] Move out M2Y from RegressionState for regr_slope and regr_intercept functions (prestodb#25475) (prestodb#25748)
Summary:
## Context
Currently we don't enforce intermediate/return type are the same in
Coordinator and Prestissimo Worker.
Velox creates vectors for intermediate/return results based on a plan
that comes from Coordinator. Then Prestissimo tries to use those vector
and not crash.
In practise we had a crash some time ago due to such a mismatch
(D74199165).
And I added validation to Velox to catch such kind of mismatches early:
facebookincubator/velox#13322
But we wasn't able to enable it in prod, because the validation failed
for "regr_slope" and "regr_intercept" functions.
## What's changed?
In this diff I'm fixing "regr_slope" and "regr_intercept" intermediate
types. Basically in Java `AggregationState` for all these functions is
the same:
```
AggregationFunction("regr_slope")
AggregationFunction("regr_intercept")
AggregationFunction("regr_sxy")
AggregationFunction("regr_sxx")
AggregationFunction("regr_syy")
AggregationFunction("regr_r2")
AggregationFunction("regr_count")
AggregationFunction("regr_avgy")
AggregationFunction("regr_avgx")
```
But in Prestissimo the state storage is more optimal:
```
AggregationFunction("regr_slope")
AggregationFunction("regr_intercept")
```
These 2 aggregation functions don't have M2Y field. And this is more
efficient, because we don't waste memory and CPU on the field, that
aren't needed.
So I moved M2Y to extended class, the same as it works in Velox:
https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/aggregates/CovarianceAggregates.cpp?fbclid=IwY2xjawLRTetleHRuA2FlbQIxMQBicmlkETFiT0N3UFR0M2VKOHl6MHRhAR6KRQ1VUQdCkZXzwj14sMQrVZ-R9QBH1utuGJb5U_lyGzDwt8PwV317QRVNJg_aem_-ePxZ-fHO5MNgfUmayVJFA#L326-L337
No major changes, mostly just reorganized the code.
## Test plan
I tested `REGR_SLOPE`, `REGR_INTERCEPT` and `REGR_R2` functions since
they are heavily used in prod and cover both cases: with and without M2Y
field.
What my test looked like. For all 3 `REGR_*` functions I found some prod
queries, then:
1. Ran them on prev Java build
2. Ran them on new (with this PR) Java build
3. Ran them on prev Prestissimo build
4. Ran them on new (with this PR) Prestissimo build
And compared the output results. They all were identical.
With this manual test we covered `Coordinator -> Java Worker` and
`Coordinator -> Prestissimo Worker` integrations.
## Next steps
In this diff I'm trying to apply the same optimization to Java. With
this fix, the signatures will become the same in Java and Prestissimo
and we will be able to enable the validation
Differential Revision: D77625566
== NO RELEASE NOTES ==1 parent 9c5004f commit 8c7f2a8
File tree
8 files changed
+345
-214
lines changed- presto-main-base/src/main/java/com/facebook/presto
- metadata
- operator/aggregation
- state
8 files changed
+345
-214
lines changedLines changed: 4 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
67 | 67 | | |
68 | 68 | | |
69 | 69 | | |
| 70 | + | |
70 | 71 | | |
71 | 72 | | |
72 | 73 | | |
| |||
84 | 85 | | |
85 | 86 | | |
86 | 87 | | |
| 88 | + | |
87 | 89 | | |
88 | 90 | | |
89 | 91 | | |
| |||
744 | 746 | | |
745 | 747 | | |
746 | 748 | | |
| 749 | + | |
747 | 750 | | |
| 751 | + | |
748 | 752 | | |
749 | 753 | | |
750 | 754 | | |
| |||
Lines changed: 21 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
| |||
145 | 146 | | |
146 | 147 | | |
147 | 148 | | |
148 | | - | |
149 | 149 | | |
150 | 150 | | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
151 | 157 | | |
152 | 158 | | |
153 | 159 | | |
| |||
189 | 195 | | |
190 | 196 | | |
191 | 197 | | |
192 | | - | |
| 198 | + | |
193 | 199 | | |
194 | 200 | | |
195 | 201 | | |
196 | 202 | | |
197 | | - | |
| 203 | + | |
198 | 204 | | |
199 | 205 | | |
200 | 206 | | |
| |||
311 | 317 | | |
312 | 318 | | |
313 | 319 | | |
314 | | - | |
315 | 320 | | |
316 | 321 | | |
317 | 322 | | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
318 | 335 | | |
319 | 336 | | |
320 | 337 | | |
| |||
Lines changed: 0 additions & 103 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | 27 | | |
31 | | - | |
32 | 28 | | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | 29 | | |
37 | 30 | | |
38 | 31 | | |
| |||
78 | 71 | | |
79 | 72 | | |
80 | 73 | | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | | - | |
115 | | - | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | | - | |
139 | | - | |
140 | | - | |
141 | | - | |
142 | | - | |
143 | | - | |
144 | | - | |
145 | | - | |
146 | | - | |
147 | | - | |
148 | | - | |
149 | | - | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | | - | |
156 | | - | |
157 | | - | |
158 | | - | |
159 | | - | |
160 | | - | |
161 | | - | |
162 | | - | |
163 | | - | |
164 | | - | |
165 | | - | |
166 | | - | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | 74 | | |
Lines changed: 149 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
0 commit comments