[fix](fe) Accept foldable gram numbers in ngram_search

[mrhhsg]

What problem does this PR solve?

Issue Number: None

Problem Summary:

ngram_search('abc', 'abc', 3) succeeds, but equivalent constant expressions such as 1 + 2, CAST('3' AS INT), and ABS(-3) are rejected before normal constant folding. Depending exclusively on FE evaluator coverage also rejects valid expressions such as crc32('abc') % 3 + 1.

Checking newly admitted constants only in BE execute_impl is insufficient: a NULL gram short-circuits before that method, empty projections never execute it, and NULL text can cause FE to eliminate the entire function call.

• Check constness and integral type separately from evaluator coverage. During function binding, evaluate the INT argument using FE when possible and the existing BE constant-evaluation RPC otherwise. Validate it before NULL/CSE/empty-plan rewrites, and retain the validated literal in the plan.
• This required argument validation is independent of enable_fold_constant_by_be. BE-only arguments require a planning-time RPC using the existing five-second FE wait limit (not a BE cancellation guarantee); unavailable/failed evaluation produces an explicit error rather than admitting an unvalidated value. As with existing planner BE folding, this wait may occur while planner table read locks are held.
• Required BE evaluation preserves the same recursive shouldSkipFold exclusions as ordinary folding. Expressions containing excluded functions such as sleep are rejected before translation/RPC, including under EXPLAIN, NULL propagation, or empty-plan rewrites; safe constants such as crc32 remain supported.
• On BE, validate a materialized gram before propagating NULL from any argument. Reuse existing nullable-column helpers; keep valid-input NULL semantics and the scoring algorithm unchanged.
• No new function signature, configuration, protocol field, or storage format.

Release note

Allow foldable positive integer constant expressions as the third argument of ngram_search, including eligible BE-evaluated expressions, while rejecting expressions excluded from BE constant folding and consistently rejecting NULL and nonpositive sizes even with NULL text or empty input.

Check List (For Author)

• Test:
  • Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.expression.FoldConstantRuleOnBETest,org.apache.doris.nereids.trees.expressions.functions.scalar.NgramSearchTest — 17 passed. New RPC-boundary tests fail on the previous implementation in both folding modes and pass with the exclusion gate; safe crc32 still dispatches an RPC and returns a literal.
  • Earlier Unit Test (BE sources unchanged in the latest follow-up): ./run-be-ut.sh --run --filter='function_string_test.ngram_search*' -j32 — 7 passed under ASAN.
  • Regression test: test_ngram_search_foldable_gram and existing test_string_function — 2 suites passed on an isolated worktree ASAN cluster. Golden output was previously generated with -forceGenOut and remains unchanged; the latest follow-up only adds error cases. Coverage includes both BE-fold modes, NULL/zero/negative grams, NULL text/pattern, CSE-shaped expressions, zero rows, WHERE false, LIMIT 0, and excluded sleep expressions under SELECT/EXPLAIN/NULL text in both folding modes.
  • Manual test: 10 new SQL probes with SQL cache disabled verified safe constants and rejected finite sleep(1)/sleep(8) grams in both modes; no sleeping BE light-pool worker remained after rejection. An earlier 26-query run covered NULL/nonpositive/empty-input behavior.
  • Build and style: ENABLE_PCH=off ./build.sh --be --fe -j32 (ASAN), FE Checkstyle, and git diff --check passed. Earlier clang-format/check-format v16 and build-hygiene checks passed for the unchanged BE sources.
  • Earlier BE static analysis limitation (BE sources unchanged in the latest follow-up): run-clang-tidy.sh --base HEAD --build-dir be/build_ASAN was attempted with the corrected local toolchain resource directory. Existing be/src/core/types.h:576 unmatched NOLINTEND aborts analysis; no emitted diagnostic intersects changed lines. This is not a clang-tidy pass.
• Behavior changed: Yes. Accept valid foldable integer gram expressions and reject invalid evaluated values before optimizer/execution short-circuits; BE-only constants must pass the existing folding exclusions and complete planning-time evaluation successfully.
• Does this need documentation: No.

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See 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?

[mrhhsg]

/review

[github-actions]

Static review outcome: changes requested (1 inline P2 finding).

Goal and scope: The PR is small and focused, and it correctly admits the tested arithmetic, cast, function, and nested positive gram expressions. However, the implementation does not satisfy the broader stated contract for foldable positive integer constants because legality depends on the limited FE evaluator rather than the executable constant-expression path.

Correctness and parallel paths: I traced bottom-up analysis, legality-before-coercion, the INT signature cast, FE and BE constant-fold modes, expression translation, nullable propagation, and BE ColumnInt32 execution. The inline finding is the only substantiated defect. Argument ordering through getArgument/withChildren is sound, volatile/context-dependent/noninteger/NULL/nonpositive inputs remain rejected, and a separate wide-integer concern was dismissed because equivalent literals already follow the same unchanged coercion semantics.

Tests and results: The new JUnit cases, regression suite, and both generated golden blocks are internally consistent, deterministic, and cover both enable_fold_constant_by_be settings. They only use functions supported by the context-free FE evaluator, so they miss the reported deterministic-but-unsupported case. This was a static-only review: no build or test was run in this review environment; author and CI claims were not treated as independent execution evidence.

Other critical checkpoints: No concurrency, locking, lifecycle/static initialization, new configuration, persistence/replay, transaction/write, FE-BE protocol/storage compatibility, memory-management, security, or observability surface is changed. The added evaluation is analysis-time only, with no per-row performance cost; no additional performance issue was found.

User focus: No additional review focus was provided.

[github-actions]

[P2] Do not use FE-evaluator coverage as the constness check

A deterministic constant tree can remain nonliteral here solely because the context-free FE evaluator does not implement its function. For example, ngram_search('abc', 'abc', crc32('abc') % 3 + 1) has a constant gram expression that is always 1-3 and is supported by the BE execution/folding path, but ExpressionEvaluator has no crc32 implementation. This call therefore leaves the tree unchanged and the next check reports gram_num support const value only, even with BE folding enabled. Please validate through a folding path that covers the executable constant-expression contract (and add this case in both fold modes), rather than treating an unchanged FE-evaluator result as proof that the argument is not a valid constant.

[mrhhsg]

Addressed in ea279c4.

Constness and integer type are now checked independently of FE evaluator coverage. FE-evaluable invalid values still fail early; otherwise normal BE execution evaluates the argument and checks that the size is positive before entering the ngram algorithm. The BE check is at the batch execution entry rather than only in open(), since CSE can materialize the constant into an intermediate slot.

Added the exact crc32('abc') % 3 + 1 case, an explicit cast, multi-row/nullable cases, and BE-only zero/negative expressions under both enable_fold_constant_by_be=false and true.

Validation: 9 FE UTs, 4 ASAN BE UTs, ASAN BE+FE build, and both the focused and existing string-function regression suites passed. Golden output was generated and then verified normally. The PR body records the pre-existing clang-tidy header blocker separately. Please re-review this revision.

[mrhhsg]

/review

[github-actions]

One correctness issue remains; see the inline comment.

Review checkpoints:

• Goal/approach: the change appropriately separates structural constness from FE evaluator coverage and adds a BE value check, but that check is not on every execution path.
• Correctness/error handling: traced FE legality and coercion, FE/BE folding, CSE materialization, nullable wrappers, projection execution, and status propagation. Invalid semantic constants can still escape validation through NULL propagation or zero-row execution.
• Tests/results: the added FE, BE, and regression cases cover valid foldable forms, non-NULL invalid values, materialized arguments, nullable rows, and both fold modes, but not the two bypasses in the inline comment. This review is static-only: the runner prohibited builds and tests, so the author's reported validation was not independently executed.
• Compatibility/performance: signatures, persistence, protocols, configuration, and shared state are unchanged; the added comparison is constant-time. No separate rolling-upgrade, concurrency, lifecycle, resource, or observability issue was found.

All six changed files were reviewed. Three rounds converged with no additional valuable findings, and all candidates were merged, dismissed with evidence, or fenced as duplicates.

[github-actions]

[P2] Validate the gram even when execution is skipped

This is the only value check, but execute_impl is not guaranteed to run. crc32('abc') % 0 remains a constant integral tree in FE and evaluates to NULL on BE, where default NULL propagation returns before this line. Also, select ngram_search(cast(number as string), 'abc', crc32('abc') % 3) from numbers("number"="0") has a row-dependent root, so the empty projection skips the function and the known-zero gram is never rejected. Literal NULL/zero grams are rejected during analysis regardless of these shapes, so newly admitted BE-only constants change the contract. Please validate semantic constants on a path that survives CSE and zero-row execution, while retaining a pre-NULL batch check for materialized nonempty slots, and cover both cases in both fold modes.

[mrhhsg]

Fixed in d487595.

The gram is now resolved and validated during function binding, before NULL propagation, CSE, or empty-plan rewrites can discard the call. FE-evaluable constants stay local; other constants use the existing BE evaluator regardless of the optional BE-fold setting. The evaluated INT literal is retained in the plan, and failed evaluation is reported rather than silently deferring validation to execution.

BE also checks materialized grams before propagating NULL from text/pattern. Regression coverage includes crc32('abc') % 0, zero/negative grams with zero rows, CSE-shaped expressions, NULL text, WHERE false, and LIMIT 0, in both fold modes. This also covers the case where NULL text previously removed the entire function on FE, which a BE-only open() fix would miss.

Validation: FE UT 12/12, ASAN BE UT 7/7, ASAN FE/BE build, and the new/existing string regression suites 2/2 passed. The prior 26 SQL probes now produce the expected outcomes. clang-tidy remains blocked by the existing unmatched NOLINTEND in core/types.h, with no emitted changed-line diagnostic.

The PR description explicitly records the additional planning RPC for BE-only grams, its existing five-second timeout, and the planner-lock waiting tradeoff. This thread is left for re-review rather than manually resolved.

[mrhhsg]

/review

[github-actions]

Static review result: request changes.

The foldable-gram design is focused and preserves the validated INT literal for execution, but the new required BE evaluator bypasses the existing unsafe-fold exclusions and can execute deliberately non-foldable work during binding.

Critical checkpoints:

• Goal and correctness: safe FE/BE-evaluable constants follow the intended contract; the blocking unsafe-dispatch case is described inline.
• Concurrency, lifecycle, and performance: long-running skipped expressions can outlive the FE timeout in the BE light pool. The already-documented planner-lock/RPC tradeoff in reply 4060476162 was duplicate-fenced rather than restated as another finding. No other shared-state or ownership defect was substantiated.
• Types, NULLs, and errors: INT normalization retains the exact validated value; reachable zero-row, constant, nullable, CSE, and null-map shapes are internally consistent. No persistence, transaction, or data-write path is involved.
• Compatibility and configuration: no wire/storage format or dynamic-configuration change was introduced; mixed-version execution continues to use the existing INT function argument. Both optional-fold settings were reviewed.
• Tests and observability: the added FE, BE, and regression cases broadly cover safe constants, invalid values, NULLs, CSE, and skipped execution, but they do not prove that unsafe expressions issue no RPC. Existing evaluator warnings/timing remain the applicable observability.
• User focus: no additional focus was supplied.

Validation scope: static review only. I did not run builds or tests, and author/CI validation claims were not independently verified.

[github-actions]

[P1] Preserve the BE-fold safety exclusions here

This direct path skips the anyMatch(shouldSkipFold) gate used by collectConst. For example, 1 + cast(sleep(3600) as int) is a deterministic integral constant, so ngram_search('abc', 'abc', 1 + cast(sleep(3600) as int)) reaches this method during binding even under WHERE false. The BE fold RPC then runs FunctionSleep; FE times out after five seconds without cancelling the future, so the BE light-pool task keeps sleeping, while shorter sleeps execute during planning and are replaced by a literal. Sleep is explicitly excluded from ordinary BE folding for exactly this timeout reason, and the same gate also protects AI/search/context-bound expressions. Please preserve those exclusions for required evaluation (reject unsafe grams without dispatching them, while still allowing safe cases such as crc32) and add a no-RPC regression for a skipped expression.

[mrhhsg]

已在 38b7a46 修复。

evaluateConstant 现在在翻译表达式、发送 RPC 之前，复用普通折叠的递归 anyMatch(shouldSkipFold) 检查。被排除的表达式保持未求值，由 ngram 参数校验明确拒绝；没有另建排除表，也没有把校验退回 batch 执行阶段。安全的 crc32 常量仍可正常求值。

补充验证：

• RPC 边界单测覆盖直接/嵌套 Sleep 和 NonNullable，断言零 RPC；同一可用 backend fixture 下，安全 crc32 对照确实发送 RPC 并得到 literal。两种 folding 设置均覆盖。这两个 no-RPC 用例在旧代码上失败，修复后通过。
• SQL 回归增加 SELECT、EXPLAIN、WHERE false 和 NULL text 下的 sleep(0) 拒绝用例，两种 folding 设置均覆盖。
• 17 个 FE 单测、2 套回归、ASAN FE/BE 构建、FE Checkstyle 和 diff 检查通过。
• 禁用 SQL cache 的有限手工测试中，sleep(1)/sleep(8) gram 现在约 14–15 ms 即被拒绝，拒绝后未观察到睡眠中的 BE light-pool worker；安全 crc32 对照正常。

本次保留的是执行准入限制，并未将 FE 的五秒等待上限描述为 BE 取消保证。

[mrhhsg]

/review

[github-actions]

Requesting changes for two FE planning/RPC issues:

• Required BE-only grams are folded one-by-one during function binding while internal-table read locks are held. The per-occurrence singleton RPCs lose batching and repeat BE runtime setup, multiplying the metadata-lock interval.
• Required validation tries only one shuffled heartbeat-alive BE, so a retryable failure of that peer rejects a valid gram despite healthy peers.

Checkpoint conclusions:

• Goal/correctness: foldable positive integral grams, pre-NULL/empty-plan validation, and recursive safety exclusions are otherwise implemented; the two issues prevent approval.
• Scope: focused FE/BE/nullability/test change.
• Concurrency/locking: no shared-state race or lock-order defect, but synchronous external RPCs under planner table read locks are a blocking issue.
• Lifecycle/memory: per-request BE fold state is RAII-cleaned; repeated setup is overhead, not a leak.
• Configuration/compatibility: no new config, protocol, signature, or storage format; the existing INT signature remains compatible.
• Parallel paths/conditions: FE-local and BE-only folds, both fold settings, CSE, NULL, zero-row/WHERE false/LIMIT 0, excluded expressions, and nullable/const BE columns were reviewed. Existing prior threads are fixed at this head and are not duplicated here.
• Tests/results: the new FE/BE/regression coverage and golden rows were inspected; missing coverage corresponds to multi-fold batching/lock ownership and healthy-peer fallback.
• Observability: existing logging/profile behavior is retained; no new metric requirement was identified.
• Transactions/persistence/writes: not applicable.
• Performance: the per-occurrence RPC fan-out is material because it serializes network and BE setup inside the metadata-lock interval.
• User focus: no additional focus was provided.
• Validation: static review only, as required by the review prompt; no builds or tests were run, and author-reported validation was not independently executed here.

[github-actions]

[P1] Batch required folds before binding under table locks

For a cold plan over an internal table, collectAndLockTable acquires read locks before analyze(), and those locks are released only after planning. Every BE-only gram reaching this line then performs its own synchronous singleton RPC. A single projection expression containing several safe ngram_search(..., crc32(...) % 3 + 1) calls therefore creates N serialized RPCs and N fresh BE fold executors/runtime states before CSE or ordinary batched folding can run. Successful-but-slow calls can multiply the same metadata-lock interval, blocking DDL; even healthy calls add repeated network/setup cost. This is distinct from the already disclosed single planning RPC. Please collect/batch these required constants outside table-lock ownership (or evaluate them locally) and add multi-expression/lock-ownership coverage.

[github-actions]

[P2] Retry another healthy peer before rejecting the gram

This required path turns evalOnBE's best-effort miss into an analysis error, but evalOnBE shuffles the heartbeat-alive IDs and tries only backendIds.get(0). A selected BE whose BRPC endpoint is restarting, whose light pool rejects the request, or whose transport fails returns the original expression; withFoldedGramNumber then rejects a valid safe gram such as crc32('abc') % 3 + 1 even when other BEs can answer. Same-address channel retries do not provide peer fallback. Please retry remaining compatible peers for retryable endpoint/transport/overload failures within one shared overall deadline (not five seconds per peer, and not deterministic expression failures), and cover first-peer failure followed by second-peer success.