Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
github-actions[bot] commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5542136710 PR approved by at least one committer and no changes requested. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
github-actions[bot] commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5542136140 PR approved by anyone and no changes requested. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
csun5285 merged PR #67431: URL: https://github.com/apache/doris/pull/67431 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
hello-stephen commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5541491988 # BE Regression && UT Coverage Report Increment line coverage `100.00% (17/17)` :tada: [Increment coverage report](http://coverage.selectdb-in.cc/coverage/67431_b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b_merge/increment_report/index.html) [Complete coverage report](http://coverage.selectdb-in.cc/coverage/67431_b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b_merge/report/index.html) | Category | Coverage | |---|| | Function Coverage | 76.29% (34655/45427) | | Line Coverage | 61.36% (391122/637438) | | Region Coverage | 57.56% (328947/571459) | | Branch Coverage | 58.40% (14/256842) | -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
hello-stephen commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5538235083 ClickBench: Total hot run time: 14.67 s ``` machine: 'aliyun_ecs.c7a.8xlarge_32C64G' scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools ClickBench test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false query1 0.010.000.00 query2 0.080.040.04 query3 0.240.100.10 query4 1.600.110.11 query5 0.180.180.16 query6 1.230.690.67 query7 0.040.010.00 query8 0.050.030.03 query9 0.300.210.22 query10 0.350.330.34 query11 0.160.110.12 query12 0.150.120.12 query13 0.310.320.31 query14 0.450.450.45 query15 0.350.350.35 query16 0.210.220.24 query17 0.610.690.73 query18 0.180.160.17 query19 1.241.211.12 query20 0.010.010.01 query21 15.44 0.170.13 query22 5.050.050.04 query23 16.19 0.240.11 query24 3.060.320.26 query25 0.120.040.03 query26 0.820.150.12 query27 0.030.030.02 query28 3.660.510.29 query29 12.47 3.162.58 query30 0.250.120.11 query31 2.750.380.17 query32 3.460.310.22 query33 1.361.351.62 query34 15.36 2.221.81 query35 1.761.751.73 query36 0.450.300.29 query37 0.060.040.04 query38 0.040.030.02 query39 0.030.030.02 query40 0.110.070.07 query41 0.070.020.02 query42 0.040.020.03 query43 0.030.030.03 Total cold run time: 90.36 s Total hot run time: 14.67 s ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
hello-stephen commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5538188854 TPC-DS: Total hot run time: 81774 ms ``` machine: 'aliyun_ecs.c7a.8xlarge_32C64G' scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools TPC-DS sf100 test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false query5 4250408 327 327 query6 384 136 120 120 query7 4969430 236 236 query8 280 126 117 117 query9 8682288328662866 query10 388 218 180 180 query11 53781022892 892 query12 115 69 70 69 query13 1193450 335 335 query14 6167216120642064 query14_11958195319391939 query15 176 120 110 110 query16 914 367 339 339 query17 791 463 367 367 query18 2332343 246 246 query19 169 139 111 111 query20 71 71 73 71 query21 198 101 88 88 query22 5449534653645346 query23 6785614159285928 query23_16263625860546054 query24 72881080764 764 query24_1796 778 780 778 query25 447 321 258 258 query26 1242247 128 128 query27 2779437 266 266 query28 4656151314911491 query29 926 455 362 362 query30 252 158 131 131 query31 812 400 331 331 query32 126 72 79 72 query33 453 217 192 192 query34 997 830 512 512 query35 410 405 351 351 query36 561 607 531 531 query37 119 81 71 71 query38 989 855 796 796 query39 495 509 475 475 query39_1455 457 453 453 query40 198 92 79 79 query41 59 60 58 58 query42 78 73 72 72 query43 239 237 211 211 query44 1031554 556 554 query45 115 105 102 102 query46 747 815 562 562 query47 749 753 700 700 query48 302 284 227 227 query49 545 233 183 183 query50 716 260 190 190 query51 8214801082428010 query52 66 70 57 57 query53 194 200 144 144 query54 216 199 161 161 query55 70 62 71 62 query56 261 159 168 159 query57 680 660 651 651 query58 202 159 159 159 query59 1196123710921092 query60 234 180 167 167 query61 116 111 141 111 query62 347 200 172 172 query63 169 133 140 133 query64 2678694 574 574 query65 1573153316001533 query66 1836261 206 206 query67 9979995197169716 query68 29951191704 704 query69 353 226 203 203 query70 671 619 622 619 query71 250 181 162 162 query72 2347170515741574 query73 663 643 343 343 query74 2000122011211121 query75 11761094960 960 query76 2389725 554 554 query77 248 254 203 203 query78 4220384332323232 query79 1175819 547 547 query80 1203310 286 286 query81 489 156 133 133 query82 625 122 95 95 query83 278 204 196 196 query84 292 115 90 90 query85 807 363 302 302 query86 396 172 172 172 query87 1022953 885 885 query88 2755209421242094 query89 286 197 173 173 query90 1995126 131 126 query91 139 126 101 101 query92 77 69 69 69 query93 12571090644 644 query94 636 258 209 209 query95 507 265 222 222 query96 815 559 260 260 query97 1050105110561051 query98 143 134 132 132 query99 427 337 321 321 Total cold run time: 176637 ms Total hot run time: 81774 ms ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
hello-stephen commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5538123042 TPC-H: Total hot run time: 16886 ms ``` machine: 'aliyun_ecs.c7a.8xlarge_32C64G' scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools Tpch sf100 test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false -- Round 1 -- q1 17615 302430273024 q2 2082262 227 227 q3 10244 896 511 511 q4 4672247 200 200 q5 7680676 385 385 q6 136 116 94 94 q7 542 538 380 380 q8 9263957 883 883 q9 3442242523822382 q10 6493847 695 695 q11 397 197 178 178 q12 617 258 209 209 q13 18113 151111591159 q14 157 146 130 130 q15 q16 429 394 362 362 q17 1384835 823 823 q18 3086226122612261 q19 1268878 771 771 q20 373 290 197 197 q21 5586178819001788 q22 319 269 227 227 Total cold run time: 93898 ms Total hot run time: 16886 ms - Round 2, with runtime_filter_mode=off - q1 3368329432873287 q2 503 381 367 367 q3 2232225521962196 q4 11791155886 886 q5 2178214621012101 q6 168 117 85 85 q7 1013910 864 864 q8 1573139613861386 q9 3112310131023101 q10 1855178416351635 q11 349 266 245 245 q12 450 434 350 350 q13 1462150911421142 q14 172 169 157 157 q15 q16 380 392 364 364 q17 3645329331873187 q18 4773438646824386 q19 849 815 912 815 q20 1015958 812 812 q21 3843311032943110 q22 387 351 326 326 Total cold run time: 34506 ms Total hot run time: 30802 ms ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
csun5285 commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5537580661 run buildall -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
csun5285 commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5537579270 /review -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
csun5285 commented on code in PR #67431:
URL: https://github.com/apache/doris/pull/67431#discussion_r3932098265
##
be/src/storage/index/zone_map/zone_map_index.cpp:
##
@@ -65,6 +80,19 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
auto field_type = data_type->get_storage_field_type();
// min value and max value are valid if has_not_null is true
if (zone_map.has_not_null()) {
+if (!zone_map_info.pass_all) {
+parse_bound(zone_map.min(), zone_map_info.min_value);
Review Comment:
允许这种情况的 zone map 失效
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
github-actions[bot] commented on code in PR #67431:
URL: https://github.com/apache/doris/pull/67431#discussion_r3911576164
##
be/src/storage/index/zone_map/zone_map_index.cpp:
##
@@ -280,6 +309,11 @@ Status
TypedZoneMapIndexWriter::finish(io::FileWriter* file_writer,
index_meta->set_type(ZONE_MAP_INDEX);
ZoneMapIndexPB* meta = index_meta->mutable_zone_map_index();
// store segment zone map
+if (_segment_zone_map.has_not_null &&
+is_reversed(_segment_zone_map.min_value, _segment_zone_map.max_value,
+_data_type->get_storage_field_type())) {
+_segment_zone_map.pass_all = true;
Review Comment:
[P1] Keep segment metadata readable by older BEs
This starts writing segment-level `pass_all=true`, which makes `to_proto()`
clear both bounds. Released 4.1 readers do not have the
`segment_zone_maps_can_answer_agg()` fence added in #67341: they still select
`VStatisticsIterator` for pushed-down MIN/MAX, skip parsing the cleared strings
because `pass_all` is set, and then insert both bounds. For a non-nullable
FLOAT/DOUBLE all-NaN (or single-infinity) segment, one side remains a default
TYPE_NULL `Field`, so an older BE reading a segment produced by an upgraded BE
can consume uninitialized storage and return a wrong aggregate during rolling
upgrade. This is reachable through shared cloud segments and replica
snapshot/clone. Please keep the legacy segment encoding until every supported
reader fences `pass_all`, or version-gate this on-disk transition. The
reader-side reversal check can still repair existing metadata, and page-level
`pass_all` is already established.
##
be/src/storage/index/zone_map/zone_map_index.cpp:
##
@@ -65,6 +80,19 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
auto field_type = data_type->get_storage_field_type();
// min value and max value are valid if has_not_null is true
if (zone_map.has_not_null()) {
+if (!zone_map_info.pass_all) {
+parse_bound(zone_map.min(), zone_map_info.min_value);
Review Comment:
[P2] Do not invalidate flag-overridden legacy bounds
Both bounds are not semantically required when a flag replaces one side. For
example, older 4.0/4.1 metadata can store `min="-1.797693134862316e+308",
max="1", has_negative_inf=true` for `{-DBL_MAX, 1, -inf}`. The current serde
rejects that historical min as infinity, but before this change it was
deliberately skipped and replaced with `-inf`, leaving a sound `[-inf, 1]`
range. Parsing it here first now sets `pass_all` and disables pruning for the
affected page/segment map. The max side has the same regression with `+inf` or
NaN, including nullable DOUBLE. Please keep parse failure fatal to pruning only
for required sides; parse a flag-overridden side best-effort for reversal
detection, and add legacy `.16g` tests for both sides.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
csun5285 commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5505399091 /review -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [fix](zonemap) Treat reversed zone map bounds as invalid [doris]
hello-stephen commented on PR #67431: URL: https://github.com/apache/doris/pull/67431#issuecomment-5505397028 Thank you for your contribution to Apache Doris. Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR). Please clearly describe your PR: 1. What problem was fixed (it's best to include specific error reporting information). How it was fixed. 2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be. 3. What features were added. Why was this function added? 4. Which code was refactored and why was this part of the code refactored? 5. Which functions were optimized and what is the difference before and after the optimization? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
