Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite #3553

songkant-aws · 2025-04-16T03:37:12Z

Description

Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite

Related Issues

Resolves #3552

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Songkan Tang <[email protected]>

qianheng-aws · 2025-04-16T08:46:26Z

ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java

+                .toString()
+                .toLowerCase(Locale.ROOT);
+    if (patternMethod.equalsIgnoreCase(PatternMethod.SIMPLE_PATTERN.getName())) {
+      return new Parse(ParseMethod.PATTERNS, sourceField, pattern, arguments);


Shall we rename the Parse node and change its doc? It's weird to use a node which says /** AST node represent Parse with regex operation. */ in its doc for PatternCommand.

Looks like our definition of UnresolvedPlan is something between AST and LogicalPlan. Most are more like AST(e.g. Parse, Rename, Eval), while others are more like LogicalPlan(e.g. Window, Aggregation, Project, Filter).

Under current convention, seems we cannot figure out a proper name for it or determine which group it belong to.

I would prefer not doing this refactoring in this PR to introduce minimum change. I left comment to call out why do we keep that Parse plan for legacy patterns method. And call out there is opportunity of refactoring it into just Project operator.

qianheng-aws · 2025-04-16T09:10:33Z

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

    RexNode sourceField = rexVisitor.analyze(node.getSourceField(), context);
    ParseMethod parseMethod = node.getParseMethod();
    java.util.Map<String, Literal> arguments = node.getArguments();
    String pattern = (String) node.getPattern().getValue();
+    // TODO: Support Grok parse method
+    Pair<SqlOperator, String> operatorPatternPair =


Could this be registered as functions in PPLFuncImpTable？Seems we will have more method in future and it should be a true function in Calcite.

Done. Registered related calcite REGEXP functions into PPLFuncImpTable

Signed-off-by: Songkan Tang <[email protected]>

LantaoJin · 2025-04-18T08:10:53Z

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java

+  REGEXP_EXTRACT(FunctionName.of("regexp_extract")),
+  REGEXP_REPLACE_2(FunctionName.of("regexp_replace_2")),


Are these functions all existing UDF in V2?

As discussed, change them to internal functions with additional boolean to describe them.

LantaoJin · 2025-04-18T08:15:43Z

ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLPatternsTest.java

+    RelNode root = getRelNode(ppl);
+
+    String expectedLogical =
+        "LogicalProject(ENAME=[$1], patterns_field=[REGEXP_REPLACE($1, '[A-H]')])\n"


I do think the '[A-H]' here should change to '[A-H]':VARCHAR if #3558 merged first Just call out for reminding.

Done. In visitParse, it's changed to makeLiteral(pattern, VARCHAR).

LantaoJin · 2025-04-18T08:21:53Z

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

    RexNode sourceField = rexVisitor.analyze(node.getSourceField(), context);
    ParseMethod parseMethod = node.getParseMethod();
    java.util.Map<String, Literal> arguments = node.getArguments();
-    String pattern = (String) node.getPattern().getValue();
+    String patternValue = (String) node.getPattern().getValue();


patterns is a command, I didn't see the implementation code visitPatterns here? How PatternsIT works?

It will be changed in part II. Part I just reverts previous changes to V2. V2's patterns representation is Parse.

Signed-off-by: Songkan Tang <[email protected]>

qianheng-aws

Could you please add tests for explain on patterns in ExplainIT as well? May need 2 tests for cases w/wo stats command after patterns.

As discussed offline, it only supports pushdown for patterns with stats command after it. It would help us verify the feature of pushing down patterns on Calcite in the future.

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws · 2025-04-23T03:17:03Z

Could you please add tests for explain on patterns in ExplainIT as well? May need 2 tests for cases w/wo stats command after patterns.

As discussed offline, it only supports pushdown for patterns with stats command after it. It would help us verify the feature of pushing down patterns on Calcite in the future.

Sure, I added suggested two cases. But I feel that IT is a bit strange. It's better to assert on actual plan change. The pushed down script is encoded string that is not readable.

Signed-off-by: Songkan Tang <[email protected]>

LantaoJin · 2025-04-28T07:50:38Z

Please update visitWindow to visitPatterns in Analyzer.

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws · 2025-04-28T09:51:22Z

Please update visitWindow to visitPatterns in Analyzer.

Done. And fixed failed IT.

LantaoJin

Could you add testPatterns in PPLQueryDataAnonymizerTest

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws · 2025-04-29T05:50:49Z

Could you add testPatterns in PPLQueryDataAnonymizerTest

Added testPatterns for SIMPLE_PATTERN method. The BRAIN method will be added in another PR because it needs some signature change of unresolved plan.

songkant-aws added 2 commits April 16, 2025 11:31

Revert simple_pattern window function change to recover pushdown ability

c06d3c3

Signed-off-by: Songkan Tang <[email protected]>

Add SIMPLE_PATTERN patterns command support based on parse command

39d99e6

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, mengweieric, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224, LantaoJin, noCharger and qianheng-aws as code owners April 16, 2025 03:37

qianheng-aws reviewed Apr 16, 2025

View reviewed changes

LantaoJin added the calcite calcite migration releated label Apr 16, 2025

Address minor comments

81d0606

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws requested a review from qianheng-aws April 17, 2025 08:41

LantaoJin reviewed Apr 18, 2025

View reviewed changes

LantaoJin added calcite calcite migration releated and removed calcite calcite migration releated labels Apr 18, 2025

LantaoJin reviewed Apr 18, 2025

View reviewed changes

songkant-aws added 4 commits April 21, 2025 10:33

Address comments part 2

8438c8e

Signed-off-by: Songkan Tang <[email protected]>

Make allowCast for pattern VARCHAR literal

f8e8945

Signed-off-by: Songkan Tang <[email protected]>

Fix spotless

48dcf64

Signed-off-by: Songkan Tang <[email protected]>

Minor ut failure fix

828685c

Signed-off-by: Songkan Tang <[email protected]>

qianheng-aws reviewed Apr 22, 2025

View reviewed changes

Add patterns command push down cases in ExplainIT

1743d16

Signed-off-by: Songkan Tang <[email protected]>

Fix patterns ExplainIT failure

b0cf346

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws added 3 commits April 28, 2025 16:11

Merge branch 'main' into calcite-patterns-command-revert-stream-pattern

1f3b035

Rename visitWindow to visitPatterns

6e715ea

Signed-off-by: Songkan Tang <[email protected]>

Correct ExplainIT expected plan for patterns

e71f354

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws requested review from LantaoJin and qianheng-aws April 28, 2025 09:51

LantaoJin approved these changes Apr 29, 2025

View reviewed changes

LantaoJin requested changes Apr 29, 2025

View reviewed changes

Add patterns anonymizer test for SIMPLE_PATTERN

07a0f37

Signed-off-by: Songkan Tang <[email protected]>

LantaoJin mentioned this pull request Apr 29, 2025

Support eventstats command with Calcite #3585

Merged

7 tasks

LantaoJin approved these changes Apr 29, 2025

View reviewed changes

qianheng-aws approved these changes Apr 30, 2025

View reviewed changes

LantaoJin merged commit 788da98 into opensearch-project:main Apr 30, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite #3553

Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite #3553

songkant-aws commented Apr 16, 2025 •

edited

Loading

qianheng-aws Apr 16, 2025 •

edited

Loading

songkant-aws Apr 17, 2025 •

edited

Loading

qianheng-aws Apr 16, 2025

songkant-aws Apr 17, 2025

LantaoJin Apr 18, 2025

songkant-aws Apr 21, 2025

LantaoJin Apr 18, 2025

songkant-aws Apr 21, 2025

LantaoJin Apr 18, 2025

songkant-aws Apr 21, 2025

qianheng-aws left a comment •

edited

Loading

songkant-aws commented Apr 23, 2025

LantaoJin commented Apr 28, 2025

songkant-aws commented Apr 28, 2025

LantaoJin left a comment

songkant-aws commented Apr 29, 2025 •

edited

Loading

		REGEXP_EXTRACT(FunctionName.of("regexp_extract")),
		REGEXP_REPLACE_2(FunctionName.of("regexp_replace_2")),

Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite #3553

Revert stream pattern method in V2 and implement SIMPLE_PATTERN in Calcite #3553

Conversation

songkant-aws commented Apr 16, 2025 • edited Loading

Description

Related Issues

Check List

qianheng-aws Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

songkant-aws Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qianheng-aws left a comment • edited Loading

Choose a reason for hiding this comment

songkant-aws commented Apr 23, 2025

LantaoJin commented Apr 28, 2025

songkant-aws commented Apr 28, 2025

LantaoJin left a comment

Choose a reason for hiding this comment

songkant-aws commented Apr 29, 2025 • edited Loading

songkant-aws commented Apr 16, 2025 •

edited

Loading

qianheng-aws Apr 16, 2025 •

edited

Loading

songkant-aws Apr 17, 2025 •

edited

Loading

qianheng-aws left a comment •

edited

Loading

songkant-aws commented Apr 29, 2025 •

edited

Loading