PPL patterns command (BRAIN + SIMPLE) on the analytics-engine route

[ahkcs]

What

Wires PPL patterns (BRAIN + SIMPLE methods, label + aggregation modes) through the analytics-engine route, including the OpenSearch Dashboards BRAIN-label panel query:

patterns message method=brain mode=label
| stats count() as pattern_count, take(message, 1) as sample_logs by patterns_field
| sort - pattern_count
| fields patterns_field, pattern_count, sample_logs

Companion to opensearch-project/sql#5467 (UnifiedQueryContext PATTERN_* defaults + dashboard IT + planner unit test). Both PRs are required.

Changes

Rust / DataFusion (sandbox/plugins/analytics-backend-datafusion)

• BRAIN classifier ported to Rust (scaffold → full classifier → scalar eval functions → PATTERN_PARSER UDF wiring).
• internal_pattern window UDF (BRAIN label mode) + aggregate UDF (BRAIN aggregation mode).
• Per-field scalar UDFs pattern_parser_get_pattern / pattern_parser_get_tokens — workaround for DataFusion's substrait consumer's "Direct reference StructField with child not supported".
• SAFE_CAST / ITEM overloads for ARRAY + MAP returns; FORCE_NULLABLE on LOCAL_TAKE_OP / LOCAL_ARRAY_AGG_OP.

Analytics-engine planner (sandbox/plugins/analytics-engine)

• PplWindowCallRewriter — bottom-up RelNode shuttle that retypes RexInputRefs before copy() so the BRAIN window-UDF result type propagates through enclosing Projects.
• OpenSearchAggregateSplitRule — skip PARTIAL/FINAL split when STATE_EXPANDING aggregates (TAKE/FIRST/LAST/LIST/VALUES) are present (avoids argList-shift crash on take(field, 1) in stats).
• ArrowValues.toJavaValue — recursively unwrap Arrow Text so nested Map<String, List<String>> returns hand the SQL layer pure Java types.

DataFusion regexp_replace adapter (RegexpReplaceAdapter)

• Append "g" flag to every 3-arg REGEXP_REPLACE going to DataFusion. DataFusion's regexp_replace defaults to first-match-only; Calcite's 3-arg form is already replace-all. The append preserves PPL contract on both backends — no SQL-core knowledge of DataFusion semantics required (companion change in feat(ppl): wire patterns command for analytics-engine dashboard route sql#5467 keeps SQL core on plain 3-arg).

Capabilities + substrait wiring

• WindowFunction.PATTERN, AggregateFunction.PATTERN.
• opensearch_window_functions.yaml registers internal_pattern window-UDF variants.
• DataFusionFragmentConvertor operator declarations + ADDITIONAL_*_SIGS for isthmus binding.

Results

Suite	Before	After
CalcitePPLDashboardPatternsIT (new)	—	1 / 1
CalcitePPLPatternsIT (analytics-engine route)	3 / 15	10 / 15

Remaining 5 / 15: 3 × BrainAggregationMode_* (planner needs LogicalCorrelate post-aggregate support) + 2 × Brain*Mode_ShowNumberedToken (substrait type wiring for _ShowNumberedToken). Tracked separately, none affect the dashboard query.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 216ae28)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The adapter now unconditionally appends a "g" flag for 3-arg REGEXP_REPLACE calls, switching them to the 4-arg PG form. If the original pattern or replacement already contained logic that assumed single-match semantics (e.g., a user explicitly avoiding global replacement), this change silently alters behavior. The Java path did not append "g" by default; this divergence may break queries that relied on single-match semantics. boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3; if (rewrittenPattern == null && rewrittenReplacement == null && !appendGlobalFlag) { return original; } RexBuilder rexBuilder = cluster.getRexBuilder(); // makeLiteral(String) sizes CHAR to the new value; reusing original type would right-pad. List<RexNode> newOperands = new ArrayList<>(original.getOperands().size() + (appendGlobalFlag ? 1 : 0)); newOperands.add(original.getOperands().get(0)); newOperands.add(rewrittenPattern != null ? rexBuilder.makeLiteral(rewrittenPattern) : patternOperand); newOperands.add(rewrittenReplacement != null ? rexBuilder.makeLiteral(rewrittenReplacement) : replacementOperand); for (int i = 3; i < original.getOperands().size(); i++) { newOperands.add(original.getOperands().get(i)); } if (appendGlobalFlag) { newOperands.add(rexBuilder.makeLiteral("g", rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true)); return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.REGEXP_REPLACE_PG_4, newOperands); } Possible Issue The new isStateExpanding check skips the PARTIAL/FINAL split for any state-expanding aggregate, but the method silently returns false if fromSqlAggFunction throws. This means an unrecognized aggregate operator (e.g., a custom UDF with SqlKind.OTHER) will bypass the split without logging or error, potentially causing incorrect execution when the aggregate actually requires single-stage semantics. private static boolean isStateExpanding(SqlAggFunction op) { try { return AggregateFunction.fromSqlAggFunction(op).getType() == AggregateFunction.Type.STATE_EXPANDING; } catch (IllegalStateException ignored) { return false; } } Possible Issue The PATTERN case constructs an explicit return type via internalPatternReturnType, but if the aggregate call's input schema changes (e.g., a column is renamed or reordered), the hardcoded field names ("pattern", "pattern_count", "tokens", "sample_logs") may not align with the actual runtime schema. The rewriter does not validate that the input matches the expected shape, so a schema mismatch will surface as a runtime Arrow type error rather than a planning-time failure. case "PATTERN" -> { // PPL declares ARRAY<MAP<VARCHAR, ANY>>; substrait can't carry ANY. targetOp = DataFusionFragmentConvertor.LOCAL_INTERNAL_PATTERN_OP; explicitReturnType = internalPatternReturnType(agg.getCluster().getTypeFactory()); } Possible Issue The tryRewritePatternParserAccess method assumes that a 3-arg pattern_parser call (BRAIN label mode) always has the window result at operand index 1. If the planner or a future rewrite changes the operand order, the method will silently extract the wrong operand and produce incorrect results. No validation confirms that operand 1 is actually the window result. List<RexNode> ppArgs = innerCall.getOperands(); if (ppArgs.size() == 3) { RexNode source = ppArgs.get(0); RexNode windowResult = ppArgs.get(1); return switch (fieldName) { case "pattern" -> windowResult; case "tokens" -> rexBuilder.makeCall( DataFusionFragmentConvertor.LOCAL_PATTERN_PARSER_GET_TOKENS_OP, List.of(windowResult, source) ); default -> null; }; Possible Issue The descendAndRewrite method checks inputRowTypeChanged by comparing row types after child rewriting, but it does not account for cases where the row type's field count matches but the field types differ in nullability or precision. If a child rewrite changes a field from nullable to non-nullable (or vice versa), the equality check may pass, and the parent's RexInputRefs will reference stale types, causing substrait conversion to fail with a type mismatch. for (int i = 0; i < oldInputs.size(); i++) { if (!oldInputs.get(i).getRowType().equals(newInputs.get(i).getRowType())) { inputRowTypeChanged = true; break; } } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 216ae28

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix ambiguous optional parameter parsing The argument parsing logic for optional parameters at indices 4 and 5 has ambiguous type detection that could misinterpret arguments. If both parameters are provided, the code tries f64 first then i64 for each position, which could incorrectly assign values if types don't match expectations. Use explicit positional semantics based on the documented argument order. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] fn from_args(values: &[ArrayRef]) -> Self { ... if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } cfg } Suggestion importance[1-10]: 8 __ Why: The current logic tries both f64 and i64 for each position, which could incorrectly assign values if the type order doesn't match expectations. The improved code enforces strict positional semantics matching the documented argument order (arg 4 = threshold_percentage as f64, arg 5 = variable_count_threshold as i64), preventing potential type confusion bugs.	Medium
General	Handle UTF-8 boundaries in substring search The bytewise substring search assumes ASCII input but the comment acknowledges non-ASCII would need char boundary handling. For UTF-8 strings with multi-byte characters, slicing at arbitrary byte offsets can panic or produce invalid results. Use str::find on the original string slices instead of byte-level comparison. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [183-191] -fn find_subsequence(haystack: &[u8], needle: &[u8]) -> Option<usize> { - if needle.is_empty() { - return Some(0); - } - if needle.len() > haystack.len() { - return None; - } - (0..=haystack.len() - needle.len()).find(|&i| &haystack[i..i + needle.len()] == needle) +fn find_subsequence_str(haystack: &str, needle: &str) -> Option<usize> { + haystack.find(needle) } Suggestion importance[1-10]: 7 __ Why: The bytewise search can panic or produce incorrect results with multi-byte UTF-8 characters. While the comment acknowledges this limitation and mentions ASCII test data, using str::find is safer and more idiomatic for production code that may encounter non-ASCII input.	Medium
	Filter nulls from pattern corpus Using unwrap_or_default() silently converts null values to empty strings, which may skew pattern detection by treating missing data as valid empty input. Consider filtering out nulls or handling them explicitly to avoid polluting the BRAIN corpus with artificial empty entries. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [136-139] let mut messages: Vec<String> = Vec::with_capacity(num_rows); for i in 0..num_rows { - messages.push(read_string_at(field_col, i).unwrap_or_default()); + if let Some(msg) = read_string_at(field_col, i) { + if !msg.is_empty() { + messages.push(msg); + } + } } Suggestion importance[1-10]: 6 __ Why: Converting nulls to empty strings via unwrap_or_default() may skew BRAIN pattern detection by treating missing data as valid empty input. Filtering out nulls and empty strings ensures the corpus only contains meaningful data, improving pattern quality.	Low
	Use sorted slice head for max frequency The find_candidate function computes max_freq by iterating the entire slice, then iterates again to find the first entry above the threshold. Since the slice is already sorted by (same_freq_count desc, word_freq desc), the maximum word_freq is the first element's word_freq. Use sorted[0].word_freq directly to avoid the redundant iteration. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/brain.rs [229-239] fn find_candidate(sorted: &[WordCombination], threshold_percentage: f64) -> WordCombination { assert!(!sorted.is_empty(), "Sorted word combinations must be non-empty"); - let max_freq = sorted.iter().map(|w| w.word_freq).max().unwrap_or(0); + let max_freq = sorted[0].word_freq; let threshold = (max_freq as f64) * threshold_percentage; for w in sorted { if (w.word_freq as f64) > threshold { return *w; } } sorted[0] } Suggestion importance[1-10]: 5 __ Why: The slice is sorted by (same_freq_count desc, word_freq desc), so sorted[0].word_freq is not necessarily the maximum word_freq across all elements (a later element with lower same_freq_count could have higher word_freq). The suggestion's premise is incorrect, but the redundant iteration could be avoided by tracking max during the sort. Score reflects the valid observation of redundancy but incorrect fix.	Low
	Reserve capacity before bulk string append The append_strings method allocates a new String for every non-null cell via to_string(). For large log corpora this creates memory pressure. Consider using strs.value(i) directly (which returns &str) and only allocating when the buffer is serialized, or use a Cow to defer cloning until necessary. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs [323-330] fn append_strings(&mut self, arr: &ArrayRef) -> Result<()> { if let Some(strs) = arr.as_any().downcast_ref::<StringArray>() { + self.buffer.reserve(strs.len()); for i in 0..strs.len() { if !strs.is_null(i) { self.buffer.push(strs.value(i).to_string()); } } return Ok(()); } ... } Suggestion importance[1-10]: 4 __ Why: Adding self.buffer.reserve(strs.len()) before the loop reduces reallocations when appending many strings. The improvement is minor (allocation overhead vs. per-element push) but valid for large log corpora.	Low

---

Previous suggestions

Suggestions up to commit 4c623c3

Category	Suggestion	Impact
Possible issue	Fix reversed type fallback logic The argument parsing logic attempts to handle both f64 and i64 types for args 4 and 5, but the fallback branches are reversed. According to the comment, arg 4 should be frequency_threshold_percentage (f64) and arg 5 should be variable_count_threshold (i64). The else if branches incorrectly assign the wrong type to the wrong field, causing silent type mismatches. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } Suggestion importance[1-10]: 9 __ Why: The fallback branches in the argument parsing logic are incorrectly reversed. When read_f64_at fails for arg 4, it tries to read it as i64 and assign to variable_count_threshold, which contradicts the comment stating arg 4 is frequency_threshold_percentage. This is a critical logic error that will cause silent type mismatches and incorrect configuration.	High
	Prevent duplicate global flag append The global flag append logic should verify that the 4-arg form doesn't already contain a "g" flag before appending. When a 3-arg call is rewritten to 4-arg with "g", a subsequent pass through the adapter would append another "g", producing "gg". sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java [59-62] boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3; if (rewrittenPattern == null && rewrittenReplacement == null && !appendGlobalFlag) { return original; } +// Prevent duplicate "g" flag on re-entry +if (original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_PG_4 && original.getOperands().size() == 4) { + RexNode flagsOperand = original.getOperands().get(3); + if (flagsOperand instanceof RexLiteral) { + String flags = ((RexLiteral) flagsOperand).getValueAs(String.class); + if (flags != null && flags.contains("g")) { + appendGlobalFlag = false; + } + } +} + Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that re-entry into the adapter after a 3-arg→4-arg rewrite would append another "g" flag, producing "gg". The fix adds a guard to check if the 4-arg form already contains "g" before setting appendGlobalFlag, preventing duplicate flags.	Medium
	Validate aggregate argument index bounds The arg0Type extraction assumes call.getArgList().get(0) is a valid input column index, but aggregate calls can reference columns beyond the input row type when the aggregate sits above a Project that computes expressions. Verify the index is within bounds before accessing getFieldList(). sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [79-98] case "LIST", "VALUES" -> { - // arg0 type distinguishes PARTIAL (raw element → array_agg) from FINAL (array → list_merge). if (call.getArgList().isEmpty()) { return call; } - RelDataType arg0Type = agg.getInput().getRowType().getFieldList().get(call.getArgList().get(0)).getType(); + int arg0Index = call.getArgList().get(0); + List<RelDataTypeField> inputFields = agg.getInput().getRowType().getFieldList(); + if (arg0Index < 0 || arg0Index >= inputFields.size()) { + return call; + } + RelDataType arg0Type = inputFields.get(arg0Index).getType(); boolean isFinal = arg0Type.getSqlTypeName() == SqlTypeName.ARRAY; if (isFinal) { targetOp = "VALUES".equals(name) ? DataFusionFragmentConvertor.LOCAL_LIST_MERGE_DISTINCT_OP : DataFusionFragmentConvertor.LOCAL_LIST_MERGE_OP; } else { targetOp = DataFusionFragmentConvertor.LOCAL_ARRAY_AGG_OP; forceDistinct = "VALUES".equals(name); explicitReturnType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly notes that call.getArgList().get(0) could reference an out-of-bounds column index when the aggregate sits above a Project. Adding bounds validation prevents potential IndexOutOfBoundsException at runtime.	Medium
General	Use first element for max frequency The max_freq calculation iterates the entire slice even though sorted is already ordered by word_freq descending (after same_freq_count). The maximum word_freq is the first element's word_freq, making the iteration redundant. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/brain.rs [229-239] fn find_candidate(sorted: &[WordCombination], threshold_percentage: f64) -> WordCombination { assert!(!sorted.is_empty(), "Sorted word combinations must be non-empty"); - let max_freq = sorted.iter().map(|w| w.word_freq).max().unwrap_or(0); + let max_freq = sorted[0].word_freq; let threshold = (max_freq as f64) * threshold_percentage; for w in sorted { if (w.word_freq as f64) > threshold { return *w; } } sorted[0] } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly observes that sorted is ordered by word_freq descending (after same_freq_count), so sorted[0].word_freq is the maximum. Replacing the iter().map().max() with direct access eliminates redundant iteration, improving efficiency.	Low
	Filter nulls from pattern corpus Using unwrap_or_default() silently converts null values to empty strings, which may skew pattern detection by treating missing data as valid empty input. Consider filtering out null rows or handling them explicitly to avoid polluting the BRAIN corpus with artificial empty-string patterns. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [136-139] let mut messages: Vec<String> = Vec::with_capacity(num_rows); for i in 0..num_rows { - messages.push(read_string_at(field_col, i).unwrap_or_default()); + if let Some(msg) = read_string_at(field_col, i) { + messages.push(msg); + } } Suggestion importance[1-10]: 6 __ Why: Converting null values to empty strings with unwrap_or_default() may affect pattern detection quality by including artificial empty entries in the corpus. However, the downstream code at line 161-163 already handles empty strings explicitly, so the impact is mitigated. Filtering nulls would be cleaner but not critical.	Low
	Pre-allocate buffer capacity for batch The append_strings method allocates a new String for every non-null row via to_string(). For large datasets this creates significant memory pressure. Use strs.value(i) directly (which returns &str) and only allocate when pushing to the buffer. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs [323-332] fn append_strings(&mut self, arr: &ArrayRef) -> Result<()> { if let Some(strs) = arr.as_any().downcast_ref::<StringArray>() { + self.buffer.reserve(strs.len()); for i in 0..strs.len() { if !strs.is_null(i) { self.buffer.push(strs.value(i).to_string()); } } return Ok(()); } ... exec_err!("{}: expected string column, got {:?}", NAME, arr.data_type()) } Suggestion importance[1-10]: 5 __ Why: The suggestion adds self.buffer.reserve(strs.len()) to pre-allocate capacity before pushing strings, reducing reallocation overhead. While correct, the impact is moderate—Vec::push already amortizes growth, so the improvement is incremental rather than critical.	Low
	Use safer window-based substring search The bytewise substring search assumes ASCII input but may produce incorrect offsets for UTF-8 strings containing multi-byte characters. When original contains non-ASCII characters, byte offsets won't align with character boundaries, causing &original[pos..absolute] to panic or produce invalid UTF-8 slices. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [183-191] fn find_subsequence(haystack: &[u8], needle: &[u8]) -> Option<usize> { if needle.is_empty() { return Some(0); } if needle.len() > haystack.len() { return None; } - (0..=haystack.len() - needle.len()).find(|&i| &haystack[i..i + needle.len()] == needle) + haystack.windows(needle.len()).position(|window| window == needle) } Suggestion importance[1-10]: 3 __ Why: While the suggested windows().position() approach is cleaner, the existing implementation already handles the same logic correctly. The comment in the code acknowledges the ASCII limitation, and the improvement is primarily stylistic rather than fixing a bug.	Low

Suggestions up to commit 87a268c

Category	Suggestion	Impact
Possible issue	Fix ambiguous optional parameter parsing The argument parsing logic for optional parameters at indices 4 and 5 has ambiguous type detection that could assign values to wrong fields. If both parameters are provided, the code tries float first then int for each position, which doesn't match the documented ordering where arg 4 should be frequency_threshold_percentage (double) and arg 5 should be variable_count_threshold (int). sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] fn from_args(values: &[ArrayRef]) -> Self { ... if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } cfg } Suggestion importance[1-10]: 8 __ Why: The current implementation has ambiguous type detection that tries both float and int for each position, which could incorrectly assign values when both optional parameters are provided. The fix ensures arg 4 is always treated as threshold_percentage (f64) and arg 5 as variable_count_threshold (i64), matching the documented parameter ordering.	Medium
	Reject negative values in scalar conversion The signed-to-unsigned cast doesn't check for negative values before converting to usize. A negative literal (e.g. -1 for max_sample_count) would wrap to a large positive value, causing memory allocation issues or incorrect behavior. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs [273-285] fn scalar_to_usize(scalar: &ScalarValue) -> Result<usize> { match scalar { - ScalarValue::Int8(Some(v)) => Ok(*v as usize), - ScalarValue::Int16(Some(v)) => Ok(*v as usize), - ScalarValue::Int32(Some(v)) => Ok(*v as usize), - ScalarValue::Int64(Some(v)) => Ok(*v as usize), - ... + ScalarValue::Int8(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int16(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int32(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int64(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::UInt8(Some(v)) => Ok(*v as usize), + ScalarValue::UInt16(Some(v)) => Ok(*v as usize), + ScalarValue::UInt32(Some(v)) => Ok(*v as usize), + ScalarValue::UInt64(Some(v)) => Ok(*v as usize), + _ => exec_err!("{}: expected non-negative integer literal, got {scalar:?}", NAME), } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a real issue where negative literals would wrap to large positive usize values, potentially causing memory allocation failures or incorrect behavior. The proposed fix adds necessary bounds checking to reject negative inputs, improving robustness.	Medium
General	Use safer substring search method The bytewise substring search assumes ASCII input but the comment acknowledges non-ASCII would need char boundary handling. For UTF-8 strings with multi-byte characters, slicing at arbitrary byte offsets can panic or produce invalid strings when converting back to &str. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [183-191] fn find_subsequence(haystack: &[u8], needle: &[u8]) -> Option<usize> { if needle.is_empty() { return Some(0); } if needle.len() > haystack.len() { return None; } - (0..=haystack.len() - needle.len()).find(|&i| &haystack[i..i + needle.len()] == needle) + haystack + .windows(needle.len()) + .position(|window| window == needle) } Suggestion importance[1-10]: 6 __ Why: The suggested windows().position() approach is cleaner and more idiomatic Rust, though functionally equivalent to the existing implementation for the byte-level search. The improvement is primarily in code clarity and maintainability rather than correctness, as both handle the ASCII case correctly.	Low
	Validate operand structure before access The rewrite logic assumes pattern_parser calls always have the expected operand structure, but doesn't validate the operand count or types before accessing them. If a malformed call reaches this point, the code may throw an IndexOutOfBoundsException or ClassCastException. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ItemTypeRebuilder.java [62-84] private RexNode tryRewritePatternParserAccess(RexCall call, List<RexNode> operands) { RexCall innerCall; RexNode keyNode; if (call.getKind() == org.apache.calcite.sql.SqlKind.ITEM && operands.size() == 2 && operands.get(0) instanceof RexCall ppDirect - && "pattern_parser".equalsIgnoreCase(ppDirect.getOperator().getName())) { + && "pattern_parser".equalsIgnoreCase(ppDirect.getOperator().getName()) + && ppDirect.getOperands().size() >= 2) { innerCall = ppDirect; keyNode = operands.get(1); } else if ("array_element".equalsIgnoreCase(call.getOperator().getName()) + && operands.size() == 2 + && operands.get(0) instanceof RexCall mapExtract + && "map_extract".equalsIgnoreCase(mapExtract.getOperator().getName()) + && mapExtract.getOperands().size() == 2 ... Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential IndexOutOfBoundsException risk when accessing ppDirect.getOperands() without validating its size. Adding && ppDirect.getOperands().size() >= 2 would prevent crashes on malformed calls, though the existing code likely never encounters such cases in practice.	Low
	Validate array element type compatibility The FINAL-phase detection relies on arg0Type being ARRAY, but the code doesn't verify that the array's element type is compatible with the expected state shape. If the input column is an unrelated array type, the rewrite may emit an incorrect operator or return type. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [79-98] case "LIST", "VALUES" -> { if (call.getArgList().isEmpty()) { return call; } int arg0Index = call.getArgList().get(0); if (arg0Index < 0 || arg0Index >= agg.getInput().getRowType().getFieldCount()) { return call; } RelDataType arg0Type = agg.getInput().getRowType().getFieldList().get(arg0Index).getType(); boolean isFinal = arg0Type.getSqlTypeName() == SqlTypeName.ARRAY; - ... + if (isFinal) { + targetOp = "VALUES".equalsIgnoreCase(name) ? DataFusionFragmentConvertor.LOCAL_LIST_MERGE_DISTINCT_OP + : DataFusionFragmentConvertor.LOCAL_LIST_MERGE_OP; + } else { + targetOp = DataFusionFragmentConvertor.LOCAL_ARRAY_AGG_OP; + explicitReturnType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); + } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code doesn't validate the array element type before assuming it's a FINAL-phase state, but the improved_code snippet is identical to the existing_code and doesn't add any validation. The concern is valid but the proposed fix doesn't address it.	Low
	Ensure sample size consistency When max_sample_count is smaller than the partition size, BRAIN only processes a subset of messages but eval_agg is called on all rows. This could result in poor pattern matches for rows not included in the sample corpus, especially if the partition has diverse log formats. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [142-152] -let stats = parser.parse_all_log_patterns(&messages, cfg.max_sample_count); +let sample_size = cfg.max_sample_count.min(num_rows); +let sampled_messages: Vec<String> = messages.iter().take(sample_size).cloned().collect(); +let stats = parser.parse_all_log_patterns(&sampled_messages, sample_size); let candidates: Vec<AggCandidate> = stats .values() .map(|entry: &PatternEntry| AggCandidate { pattern: entry.pattern.clone(), }) .collect(); Suggestion importance[1-10]: 4 __ Why: While the suggestion correctly identifies that max_sample_count may be smaller than num_rows, the proposed change doesn't address the core concern. The BRAIN parser already handles sampling internally via its max_sample_count parameter, so explicitly limiting the input messages would be redundant and potentially alter the algorithm's behavior.	Low
	Separate no-op and flag-append paths The global flag append logic is unconditional for all 3-arg calls, but the adapter should only append "g" when the pattern/replacement rewrite didn't already switch to the 4-arg form. If a rewrite occurred and the operator is still REGEXP_REPLACE_3, the flag append is correct; otherwise the condition may cause double-appending or incorrect operator selection. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java [59-62] -boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3; - -if (rewrittenPattern == null && rewrittenReplacement == null && !appendGlobalFlag) { +if (rewrittenPattern == null && rewrittenReplacement == null) { + if (original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3) { + List<RexNode> newOperands = new ArrayList<>(original.getOperands()); + newOperands.add(rexBuilder.makeLiteral("g", rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true)); + return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.REGEXP_REPLACE_PG_4, newOperands); + } return original; } +boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3; +// ... rest of the rewrite logic + Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential logic issue where the flag-append path might interact incorrectly with the rewrite logic, but the existing code already handles this correctly by checking rewrittenPattern == null && rewrittenReplacement == null && !appendGlobalFlag before returning the original. The suggested refactoring would duplicate logic without fixing an actual bug.	Low

Suggestions up to commit 6653bde

Category	Suggestion	Impact
Possible issue	Advance position past matched delimiter After finding the next static delimiter, pos is updated to absolute (the start of the delimiter) but doesn't advance past it. This causes the subsequent static-part check to fail because original[pos..] still starts with the delimiter. The position should advance by the delimiter's length. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [160-168] -let search_region = &original_bytes[pos..]; -let next_static_bytes = next_static.as_bytes(); match find_subsequence(search_region, next_static_bytes) { None => return, Some(rel_idx) => { let absolute = pos + rel_idx; let value = &original[pos..absolute]; add_to_result(result, token_key, value.to_string()); - pos = absolute; - i += 1; + pos = absolute + next_static.len(); + i += 2; } } Suggestion importance[1-10]: 9 __ Why: Critical bug: after finding a delimiter, pos is set to the delimiter's start but doesn't advance past it. This causes the next static-part check to fail. The fix correctly advances pos by the delimiter length and skips the next iteration (i += 2), which is essential for proper pattern matching.	High
General	Add bounds check for token_order access The function accesses token_order[token_index] without verifying that token_index is within bounds. If the is_token and token_order vectors become desynchronized (e.g., due to a bug in parse_pattern), this could panic. Add a bounds check before accessing token_order. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [144-146] -pub fn extract_variables( - parse_result: &ParseResult, - original: &str, - result: &mut TokensMap, - _prefix: &str, -) { +if is_token[i] { + if token_index >= token_order.len() { + return; + } + let token_key = &token_order[token_index]; + token_index += 1; ... - while i < parts.len() { - let current_part = &parts[i]; - if is_token[i] { - let token_key = &token_order[token_index]; - token_index += 1; - if i == parts.len() - 1 { - ... - } else { - let next_static = &parts[i + 1]; - ... - } - } else { - if original[pos..].starts_with(current_part) { - pos += current_part.len(); - i += 1; - } else { - return; - } - } - } } Suggestion importance[1-10]: 8 __ Why: Accessing token_order[token_index] without bounds checking could cause a panic if the vectors become desynchronized. This is a potential runtime safety issue that should be prevented with a bounds check before array access.	Medium
	Remove ambiguous type fallback logic The argument parsing logic for optional parameters at indices 4 and 5 attempts to handle both f64 and i64 types in either position, but this creates ambiguity. According to the comment, arg 4 should be frequency_threshold_percentage (double) and arg 5 should be variable_count_threshold (int). The fallback logic could misinterpret types if the wrong type appears at the wrong index. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] fn from_args(values: &[ArrayRef]) -> Self { ... if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } cfg } Suggestion importance[1-10]: 7 __ Why: The current implementation has ambiguous fallback logic that could misinterpret argument types. The comment clearly states arg 4 should be frequency_threshold_percentage (f64) and arg 5 should be variable_count_threshold (i64), but the code tries both types at both positions. Removing the fallback improves correctness and clarity.	Medium
	Pre-allocate buffer capacity for performance The append_strings method allocates a new String for every non-null element via to_string(). For large datasets this creates significant memory pressure. Consider using strs.value(i) directly as &str and only allocating when necessary, or pre-allocating the buffer capacity based on the array length. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs [322-330] fn append_strings(&mut self, arr: &ArrayRef) -> Result<()> { if let Some(strs) = arr.as_any().downcast_ref::<StringArray>() { + self.buffer.reserve(strs.len()); for i in 0..strs.len() { if !strs.is_null(i) { self.buffer.push(strs.value(i).to_string()); } } return Ok(()); } ... Suggestion importance[1-10]: 5 __ Why: The suggestion to pre-allocate buffer capacity via self.buffer.reserve(strs.len()) is a valid performance optimization that reduces reallocation overhead when appending many strings. However, the impact is moderate (not critical) and the suggestion doesn't address the to_string() allocation concern mentioned in the rationale.	Low
	Guard against empty input corpus The parse_all_log_patterns method processes the entire log corpus in memory without any size limit. For very large datasets this can cause memory exhaustion. Consider adding a configurable corpus size limit and returning an error or truncating the input when the limit is exceeded, similar to the buffer_limit parameter mentioned in the aggregate UDF signature. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/brain.rs [101-105] pub fn parse_all_log_patterns( &mut self, log_messages: &[String], max_sample_count: usize, ) -> BrainParseStats { + if log_messages.is_empty() { + return HashMap::new(); + } let preprocessed = self.preprocess_all_logs(log_messages); ... Suggestion importance[1-10]: 4 __ Why: Adding an empty-input guard is a sensible defensive measure that avoids unnecessary work when log_messages is empty. However, the suggestion's rationale about memory exhaustion and corpus size limits is not addressed by the "improved_code" (which only checks for empty input), making the score moderate.	Low
	Validate argument list before rewrite The PATTERN case unconditionally sets explicitReturnType but doesn't verify that the aggregate call's argument list matches the expected shape. If the call has unexpected arguments or types, the rewrite may produce a malformed aggregate. Add a guard to check call.getArgList() size and types before proceeding with the rewrite. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [99-103] case "PATTERN" -> { - // PPL declares ARRAY<MAP<VARCHAR, ANY>>; substrait can't carry ANY. + if (call.getArgList().isEmpty()) { + return call; + } targetOp = DataFusionFragmentConvertor.LOCAL_INTERNAL_PATTERN_OP; explicitReturnType = internalPatternReturnType(agg.getCluster().getTypeFactory()); } Suggestion importance[1-10]: 3 __ Why: Adding an empty-argument-list guard is a reasonable defensive check, but the suggestion doesn't address the broader concern about verifying argument types. The improvement is minor and the "improved_code" only adds a single isEmpty() check without validating the expected shape.	Low

Suggestions up to commit 111578e

Category	Suggestion	Impact
Possible issue	Fix unsafe byte-to-char casting for UTF-8 Casting raw bytes to char via as char is unsafe for non-ASCII UTF-8 sequences and can produce invalid Unicode. Use std::str::from_utf8 or iterate over input.chars() instead of raw bytes to preserve UTF-8 correctness and avoid potential data corruption. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/preprocess.rs [103-125] fn apply_generic_number_filter(input: &str) -> String { - let bytes = input.as_bytes(); let mut out = String::with_capacity(input.len()); + let chars: Vec<char> = input.chars().collect(); let mut i = 0; - while i < bytes.len() { + while i < chars.len() { + let c = chars[i]; + let is_digit = c.is_ascii_digit() || c == '-' || c == '+'; + if is_digit { + if i == 0 { + out.push(c); + i += 1; + continue; + } + let prev = chars[i - 1]; + ... + } ... - if i == 0 { - out.push(c as char); - i += 1; - continue; - } - let prev = bytes[i - 1]; + } + out +} Suggestion importance[1-10]: 9 __ Why: Casting raw bytes to char via as char is unsafe for non-ASCII UTF-8 and can produce invalid Unicode. The suggestion correctly identifies a critical correctness issue and proposes using chars() iterator to preserve UTF-8 validity, preventing potential data corruption.	High
	Validate signed integer bounds before casting Casting signed integers to usize without validation can cause integer overflow or produce incorrect values when the input is negative. Add bounds checking to ensure the value is non-negative and within usize range before casting to prevent runtime errors or silent data corruption. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs [273-285] fn scalar_to_usize(scalar: &ScalarValue) -> Result<usize> { match scalar { - ScalarValue::Int8(Some(v)) => Ok(*v as usize), - ScalarValue::Int16(Some(v)) => Ok(*v as usize), - ScalarValue::Int32(Some(v)) => Ok(*v as usize), - ScalarValue::Int64(Some(v)) => Ok(*v as usize), + ScalarValue::Int8(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int16(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int32(Some(v)) if *v >= 0 => Ok(*v as usize), + ScalarValue::Int64(Some(v)) if *v >= 0 => usize::try_from(*v).map_err(|_| + datafusion::common::DataFusionError::Execution(format!("{}: value out of range", NAME))), ... } } Suggestion importance[1-10]: 8 __ Why: Casting signed integers to usize without validation can cause integer overflow or incorrect values when negative. The suggestion correctly identifies a potential runtime error and proposes bounds checking to prevent silent data corruption or crashes.	Medium
	Validate UTF-8 character boundaries The bytewise substring search and slicing assumes ASCII-compatible input but operates on UTF-8 byte boundaries without validation. If original contains multi-byte UTF-8 characters, slicing at arbitrary byte offsets (original[pos..absolute]) can panic or produce invalid UTF-8. Use character-boundary-aware indexing or validate that slice boundaries fall on valid UTF-8 boundaries. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [160-168] -pub fn extract_variables( - parse_result: &ParseResult, - original: &str, - result: &mut TokensMap, - _prefix: &str, -) { - ... - let original_bytes = original.as_bytes(); - ... - let search_region = &original_bytes[pos..]; - let next_static_bytes = next_static.as_bytes(); - match find_subsequence(search_region, next_static_bytes) { - None => return, - Some(rel_idx) => { - let absolute = pos + rel_idx; +match find_subsequence(search_region, next_static_bytes) { + None => return, + Some(rel_idx) => { + let absolute = pos + rel_idx; + if original.is_char_boundary(pos) && original.is_char_boundary(absolute) { let value = &original[pos..absolute]; - ... + add_to_result(result, token_key, value.to_string()); + pos = absolute; + i += 1; + } else { + return; } } } Suggestion importance[1-10]: 8 __ Why: The code performs byte-level slicing on UTF-8 strings without validating character boundaries, which can cause panics or invalid UTF-8 when multi-byte characters are present. Adding is_char_boundary checks prevents runtime errors and ensures correctness for non-ASCII input.	Medium
	Enforce strict parameter type checking The argument parsing logic for optional parameters at indices 4 and 5 attempts to handle both f64 and i64 types in either position, but the comment states they are ordered by argument name (frequency_threshold_percentage first, variable_count_threshold second). This ambiguous fallback logic could lead to incorrect parameter assignment if types don't match expectations. Enforce strict type checking per position or validate the argument order explicitly. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] fn from_args(values: &[ArrayRef]) -> Self { ... + // arg 4: frequency_threshold_percentage (f64) if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } + // arg 5: variable_count_threshold (i64) if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } cfg } Suggestion importance[1-10]: 7 __ Why: The current implementation tries both f64 and i64 for positions 4 and 5, which contradicts the comment stating they are ordered by name. This could lead to incorrect parameter assignment if the wrong type is passed. Enforcing strict type checking per position improves correctness and prevents subtle bugs.	Medium
General	Replace panic with error return Using assert! for input validation causes a panic on invalid input, which crashes the process. Replace with a Result return type and proper error handling so invalid thresholds are reported gracefully without terminating execution. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/brain.rs [82-87] -pub fn with_thresholds(variable_count_threshold: usize, threshold_percentage: f64) -> Self { - assert!( - (0.0..=1.0).contains(&threshold_percentage), - "threshold_percentage must be in [0.0, 1.0]" - ); - ... +pub fn with_thresholds(variable_count_threshold: usize, threshold_percentage: f64) -> Result<Self, String> { + if !(0.0..=1.0).contains(&threshold_percentage) { + return Err(format!("threshold_percentage must be in [0.0, 1.0], got {}", threshold_percentage)); + } + Ok(Self { + variable_count_threshold, + threshold_percentage, + ... + }) } Suggestion importance[1-10]: 7 __ Why: Using assert! for input validation causes a panic that crashes the process. The suggestion correctly proposes returning a Result for graceful error handling, which is a better practice for library code. However, this is a constructor-level validation and the impact depends on usage context.	Medium
	Remove redundant operand size check The condition checks original.getOperands().size() == 3 after already validating the size at the method entry (lines 70-72). This redundant check can be removed since the method returns early if size is not 3 or 4. Simplify to appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3; to avoid unnecessary computation. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java [59-62] -boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3 && original.getOperands().size() == 3; +boolean appendGlobalFlag = original.getOperator() == SqlLibraryOperators.REGEXP_REPLACE_3; if (rewrittenPattern == null && rewrittenReplacement == null && !appendGlobalFlag) { return original; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a redundant check since the method already validates operand size at entry (lines 31-33 in the new hunk). However, the redundancy is minor and doesn't affect correctness—it's a small optimization that improves code clarity.	Low
	Validate max_sample_count before parsing If max_sample_count is set to 0 or a very small value, parse_all_log_patterns may produce an empty or trivial pattern set, causing all rows to receive empty pattern strings. Validate that max_sample_count is within a reasonable range (e.g., at least 1) before invoking the parser to prevent degenerate results. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [128-146] -fn evaluate_all(&mut self, values: &[ArrayRef], num_rows: usize) -> Result<ArrayRef> { - if values.is_empty() { - return Ok(std::sync::Arc::new(StringArray::from(vec![None::<&str>; num_rows]))); - } - let cfg = WindowConfig::from_args(values); - ... - let mut parser = BrainLogParser::with_thresholds( - cfg.variable_count_threshold, - cfg.threshold_percentage, - ); - let stats = parser.parse_all_log_patterns(&messages, cfg.max_sample_count); +let cfg = WindowConfig::from_args(values); +if cfg.max_sample_count == 0 { + return Ok(std::sync::Arc::new(StringArray::from(vec![""; num_rows]))); +} +... +let mut parser = BrainLogParser::with_thresholds( + cfg.variable_count_threshold, + cfg.threshold_percentage, +); +let stats = parser.parse_all_log_patterns(&messages, cfg.max_sample_count); Suggestion importance[1-10]: 5 __ Why: While validating max_sample_count is reasonable, the suggestion to return empty strings for all rows when it's 0 may not be the desired behavior. The current code already applies .max(0) when parsing the config, and the BRAIN parser likely handles edge cases. This is a minor improvement for defensive programming.	Low

Suggestions up to commit 6058798

Category	Suggestion	Impact
Possible issue	Remove ambiguous type fallback logic The argument parsing logic for optional parameters at indices 4 and 5 attempts to handle both types in either position, which creates ambiguity. According to the comment, arg 4 should be frequency_threshold_percentage (f64) and arg 5 should be variable_count_threshold (i64). The fallback type checks in each branch will cause incorrect behavior if the types don't match expectations. sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs [207-220] fn from_args(values: &[ArrayRef]) -> Self { ... if let Some(arr) = values.get(4) { if let Some(v) = read_f64_at(arr, 0) { cfg.threshold_percentage = v; - } else if let Some(v) = read_i64_at(arr, 0) { - cfg.variable_count_threshold = v.max(0) as usize; } } if let Some(arr) = values.get(5) { if let Some(v) = read_i64_at(arr, 0) { cfg.variable_count_threshold = v.max(0) as usize; - } else if let Some(v) = read_f64_at(arr, 0) { - cfg.threshold_percentage = v; } } cfg } Suggestion importance[1-10]: 8 __ Why: The current implementation has ambiguous fallback logic that attempts to parse both f64 and i64 types for arguments 4 and 5, which contradicts the documented parameter order. This could lead to incorrect configuration when arguments are provided, as the wrong type check might succeed and assign values to the wrong field.	Medium
	Handle UTF-8 boundaries correctly The bytewise substring search assumes ASCII input but the comment acknowledges non-ASCII data requires char boundary handling. For UTF-8 strings with multi-byte characters, slicing at arbitrary byte offsets can panic or produce invalid results. Use str::find on the UTF-8 string instead of byte-level search to ensure correctness. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [183-191] -fn find_subsequence(haystack: &[u8], needle: &[u8]) -> Option<usize> { - if needle.is_empty() { - return Some(0); - } - if needle.len() > haystack.len() { - return None; - } - (0..=haystack.len() - needle.len()).find(|&i| &haystack[i..i + needle.len()] == needle) +fn find_subsequence_str(haystack: &str, needle: &str) -> Option<usize> { + haystack.find(needle) } Suggestion importance[1-10]: 7 __ Why: The bytewise search in find_subsequence can produce incorrect results or panic with non-ASCII UTF-8 input, as acknowledged in the code comments. While the current tests use ASCII data, using str::find would be more robust and correct for general UTF-8 strings.	Medium
General	Validate variable_count_threshold is positive The variable_count_threshold parameter is not validated. If a caller passes zero or an unreasonably large value, the BRAIN algorithm's group-token-set logic could produce incorrect patterns or panic on arithmetic operations. Add a range check to ensure variable_count_threshold is positive and within a sensible upper bound. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/brain.rs [82-96] pub fn with_thresholds(variable_count_threshold: usize, threshold_percentage: f64) -> Self { + assert!( + variable_count_threshold > 0, + "variable_count_threshold must be positive" + ); assert!( (0.0..=1.0).contains(&threshold_percentage), "threshold_percentage must be in [0.0, 1.0]" ); Self { variable_count_threshold, threshold_percentage, ... } } Suggestion importance[1-10]: 6 __ Why: A zero or negative variable_count_threshold would break the BRAIN algorithm's group-token-set logic (e.g., the >= threshold check in parse_log_pattern). Adding a positive-value assertion prevents silent failures and improves input validation, though the default value is already safe.	Low
	Reject duplicate optional threshold arguments The optional tail parsing loop assigns threshold_percentage and variable_count_threshold based on Arrow type, but if both a float and an int literal appear in the tail, the last one of each type wins. This could silently overwrite user-provided values if the PPL layer sends duplicate arguments. Add a guard to detect and reject duplicate assignments. [sandbox/plugins/...

[github-actions]

Persistent review updated to latest commit a4d08d8

[github-actions]

❌ Gradle check result for a4d08d8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 5380a05

[github-actions]

❌ Gradle check result for 5380a05: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit e6db8ce

[github-actions]

✅ Gradle check result for e6db8ce: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.45%. Comparing base (6105940) to head (216ae28).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21797      +/-   ##
============================================
+ Coverage     73.37%   73.45%   +0.08%     
- Complexity    75448    75532      +84     
============================================
  Files          6034     6033       -1     
  Lines        342504   342572      +68     
  Branches      49259    49276      +17     
============================================
+ Hits         251310   251637     +327     
+ Misses        71175    70947     -228     
+ Partials      20019    19988      -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 9d67da9

[github-actions]

✅ Gradle check result for 9d67da9: SUCCESS

[github-actions]

Persistent review updated to latest commit 9c7d359

[github-actions]

❌ Gradle check result for 9c7d359: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit ec6da64

[github-actions]

❌ Gradle check result for ec6da64: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 9b60303

[github-actions]

Persistent review updated to latest commit e39c9ee

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit f207f61.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/internal_pattern.rs	29	medium	The `buffer_limit` parameter is explicitly accepted in the UDAF signature for parity with Java but is documented as intentionally ignored ('currently ignored — no partial pre-aggregation'). The accumulator's `buffer` field grows without bound until `evaluate()` is called, meaning a query over a large corpus has no per-shard memory ceiling. An attacker with query access could trigger OOM by submitting a BRAIN aggregate over a sufficiently large index shard. This is a deliberate architectural shortcut, not a bug.
sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/preprocess.rs	142	low	A deliberately non-matching placeholder regex (`r"(?-u)(?:^^^^^^never^^^^^^)"`) is inserted at index 4 of the DEFAULT_FILTER_PATTERNS slice solely to maintain index-count parity with Java. The surrounding loop explicitly skips index 4. If a future change removes or reorders this skip logic, the nonsensical regex becomes active as a filter rule. The intent is documented but the pattern is fragile.
sandbox/plugins/analytics-backend-datafusion/rust/src/udwf/internal_pattern.rs	224	low	WindowConfig::from_args attempts to infer whether argument slot 4 is a float or an integer by trying both read_f64_at and read_i64_at in sequence, with no validation that exactly one type matched. If a caller passes an integer value in slot 4 and a float in slot 5, the heuristic could silently swap the semantics of threshold_percentage and variable_count_threshold, producing unexpectedly permissive or restrictive pattern grouping without any error. This is an out-of-place silent type-coercion path in security-adjacent configuration parsing.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 5bee96e

[github-actions]

Persistent review updated to latest commit ff3830f

[github-actions]

Persistent review updated to latest commit 4c093f4

[github-actions]

Persistent review updated to latest commit 6058798

[github-actions]

Persistent review updated to latest commit 111578e

[github-actions]

Persistent review updated to latest commit 6653bde

[github-actions]

Persistent review updated to latest commit 87a268c

[github-actions]

✅ Gradle check result for 87a268c: SUCCESS

[github-actions]

Persistent review updated to latest commit 4c623c3

[github-actions]

✅ Gradle check result for 4c623c3: SUCCESS

[github-actions]

Persistent review updated to latest commit 216ae28

[github-actions]

✅ Gradle check result for 216ae28: SUCCESS