Skip to content

Add isnotempty PPL function#5193

Open
lucacavenaghi97 wants to merge 1 commit intoopensearch-project:mainfrom
lucacavenaghi97:add-isnotempty-function
Open

Add isnotempty PPL function#5193
lucacavenaghi97 wants to merge 1 commit intoopensearch-project:mainfrom
lucacavenaghi97:add-isnotempty-function

Conversation

@lucacavenaghi97
Copy link

Description

Adds isnotempty(field) to PPL. Returns true when a field is not null and not empty string — logical negation of isempty(field).

Resolves #5182

Changes

  • Added ISNOTEMPTY token and parser rules in ppl, language-grammar, and async-query-core grammars
  • Added IS_NOT_EMPTY enum constant to BuiltinFunctionName
  • Registered Calcite implementation as NOT(IS_NULL(arg) OR IS_EMPTY(arg)) in PPLFuncImpTable
  • Added testIsNotEmpty integration test
  • Added docs

@github-actions
Copy link
Contributor

github-actions bot commented Feb 27, 2026

PR Reviewer Guide 🔍

(Review updated until commit 0ecc7f1)

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

Logic Duplication

The implementation of IS_NOT_EMPTY wraps the entire IS_EMPTY logic in a NOT operator. Consider refactoring to reuse the IS_EMPTY implementation directly to avoid code duplication and improve maintainability.

register(
    IS_NOT_EMPTY,
    (FunctionImp1)
        (builder, arg) ->
            builder.makeCall(
                SqlStdOperatorTable.NOT,
                builder.makeCall(
                    SqlStdOperatorTable.OR,
                    builder.makeCall(SqlStdOperatorTable.IS_NULL, arg),
                    builder.makeCall(SqlStdOperatorTable.IS_EMPTY, arg))),
    PPLTypeChecker.family(SqlTypeFamily.ANY));

@github-actions
Copy link
Contributor

github-actions bot commented Feb 27, 2026

PR Code Suggestions ✨

Latest suggestions up to 0ecc7f1
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Extract duplicated IS_EMPTY logic

The IS_NOT_EMPTY implementation duplicates the logic from IS_EMPTY. Consider
extracting the IS_EMPTY logic into a reusable method and negating it, which would
improve maintainability and reduce code duplication. This ensures both functions
stay synchronized if the empty check logic needs to change.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1256-1266]

 register(
     IS_NOT_EMPTY,
     (FunctionImp1)
         (builder, arg) ->
             builder.makeCall(
                 SqlStdOperatorTable.NOT,
-                builder.makeCall(
-                    SqlStdOperatorTable.OR,
-                    builder.makeCall(SqlStdOperatorTable.IS_NULL, arg),
-                    builder.makeCall(SqlStdOperatorTable.IS_EMPTY, arg))),
+                makeIsEmptyCall(builder, arg)),
     PPLTypeChecker.family(SqlTypeFamily.ANY));
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies code duplication between IS_NOT_EMPTY and IS_EMPTY implementations. However, the proposed makeIsEmptyCall method doesn't exist in the codebase, and the suggestion doesn't show how to implement it. While refactoring would improve maintainability, the current implementation is functionally correct and the improvement is moderate.

Low

Previous suggestions

Suggestions up to commit 88cfad4
CategorySuggestion                                                                                                                                    Impact
General
Extract common expression to variable

The isnotempty implementation duplicates the isempty logic inline. Consider
extracting the common OR expression into a variable or reusing the existing IS_EMPTY
registration to avoid code duplication and improve maintainability.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1256-1266]

 register(
     IS_NOT_EMPTY,
     (FunctionImp1)
-        (builder, arg) ->
-            builder.makeCall(
-                SqlStdOperatorTable.NOT,
-                builder.makeCall(
-                    SqlStdOperatorTable.OR,
-                    builder.makeCall(SqlStdOperatorTable.IS_NULL, arg),
-                    builder.makeCall(SqlStdOperatorTable.IS_EMPTY, arg))),
+        (builder, arg) -> {
+          RexNode isEmptyCall = builder.makeCall(
+              SqlStdOperatorTable.OR,
+              builder.makeCall(SqlStdOperatorTable.IS_NULL, arg),
+              builder.makeCall(SqlStdOperatorTable.IS_EMPTY, arg));
+          return builder.makeCall(SqlStdOperatorTable.NOT, isEmptyCall);
+        },
     PPLTypeChecker.family(SqlTypeFamily.ANY));
Suggestion importance[1-10]: 4

__

Why: While extracting the common OR expression into a variable improves readability slightly, the impact is minimal. The current implementation is already clear and the duplication is intentional for defining IS_NOT_EMPTY as the negation of IS_EMPTY. This is a minor style improvement rather than a functional or maintainability concern.

Low

Adds `isnotempty(field)` function to PPL that returns true when a field
is not null and not an empty string — the logical negation of
`isempty(field)`.

Implementation: NOT(IS_NULL(arg) OR IS_EMPTY(arg))

- ANTLR grammar: added ISNOTEMPTY token and parser rules in all three
  grammar modules (ppl, language-grammar, async-query-core)
- BuiltinFunctionName: added IS_NOT_EMPTY enum constant
- PPLFuncImpTable: registered Calcite implementation
- Integration test: testIsNotEmpty in CalcitePPLConditionBuiltinFunctionIT
- Documentation: added ISNOTEMPTY section to condition functions docs

Resolves opensearch-project#5182

Signed-off-by: Luca Cavenaghi <lucacavenaghics97@gmail.com>
@github-actions
Copy link
Contributor

Persistent review updated to latest commit 0ecc7f1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] isnotempty function

1 participant