yiguolei commented on PR #70302: URL: https://github.com/apache/doris/pull/70302#issuecomment-6758355942
Review of commit `c8d14ea15c178ab67ed6c7bfec9246771d550d24`: **REQUEST_CHANGES** — two issues to address before merging. ### 1. Blocker: the new multirow Consted unit test fails during input construction [function_geo_test.cpp:356–357](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/test/exprs/function/function_geo_test.cpp#L356-L357) `function_geo_st_contains_const_polygon` passes a seven-row dataset to `check_function` with the polygon argument marked `Consted`. The [helper](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/test/exprs/function/function_test_util.h#L343-L359) inserts all seven rows into the nested column before wrapping it in `ColumnConst`. However, [ColumnConst's constructor](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/src/core/column/column_const.cpp#L55-L60) requires exactly one nested row and throws: ```text Incorrect size of nested column in constructor of ColumnConst: 7, must be 1. ``` Consequently, this test never reaches the function's open/execute path. The later three-row `st_within` and two-row invalid-constant cases have the same construction problem. Please construct each constant from a one-row nested column with logical size N, as the adjacent across-block test does, or update the helper to insert only one nested row for `Consted` arguments while retaining the intended multirow coverage. ### 2. Major: the new retained cache buffers bypass query memory accounting and limits [functions_geo.cpp:576–579](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/src/exprs/function/geo/functions_geo.cpp#L576-L579) and [geo_types.h:224–227](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/src/exprs/function/geo/geo_types.h#L224-L227) The encoded cache key uses an ordinary `std::string`, and the new vertex cache uses ordinary nested `std::vector` containers. Neither uses a Doris tracking allocator or explicit accounting. The [jemalloc hook](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/src/runtime/memory/jemalloc_hook.cpp#L34-L39) does not perform tracker consumption; that check/accounting is in the Doris [allocator path](https://github.com/apache/doris/blob/c8d14ea15c178ab67ed6c7bfec9246771d550d24/be/src/core/allocator.cpp#L306-L310). A concrete trigger is: ```sql ST_Contains(REPEAT('x', 8388608), ST_Point(lng, lat)) ``` With nonconstant coordinates, the relation is evaluated at runtime. Previously, decoding rejected the invalid header immediately without copying the payload. Now each execution context first copies and retains an additional 8 MiB encoded key, then returns NULL. N cloned contexts therefore retain approximately N × 8 MiB of additional storage that is absent from query memory-limit enforcement. Valid polygon vertex buffers add another source of unaccounted retained memory. Please use allocator-aware ownership or checked, balanced RAII accounting for the new buffers, including both vertex-vector levels and retained capacity. Avoid or bound retaining oversized invalid encodings. These buffers are eventually released, so this is a query resource-control issue rather than a permanent leak. The THREAD_LOCAL open/clone ownership itself looks correct: original contexts open both scopes, and clones initialize their own thread-local state. No additional result regression was established in the reviewed boundary-box/tolerance logic. Validation scope: source review of all six changed files with two rounds of cross-checking; no build, unit-test execution, benchmark, or cluster run was performed. The test failure and memory-limit trigger above are source-traced conclusions, not claims of executed reproductions. -- 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]
