generated from amazon-archives/__template_Custom
-
Notifications
You must be signed in to change notification settings - Fork 179
Support spath command to extract all fields #4822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ykmr1224
wants to merge
6
commits into
opensearch-project:feature/permissive
Choose a base branch
from
ykmr1224:dynamic-spath-append
base: feature/permissive
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
972a480
Support spath command without path parameter
ykmr1224 08703dd
Fix test failure
ykmr1224 3020586
Refactor append logic
ykmr1224 0b7ad9f
Fix test
ykmr1224 28cbf06
Add type mapping for UNDEFINED
ykmr1224 48f5633
Address comments
ykmr1224 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,14 +8,32 @@ | |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| /** Core logic for `mvappend` command to collect elements from list of args */ | ||
| public class MVAppendCore { | ||
| /** | ||
| * Core logic for `mvappend` and internal `append` function to collect elements from list of args. | ||
| */ | ||
| public class AppendCore { | ||
|
|
||
| /** | ||
| * Collect non-null elements from `args`. If an item is a list, it will collect non-null elements | ||
| * of the list. See {@ref MVAppendFunctionImplTest} for detailed behavior. | ||
| * of the list. See {@ref AppendFunctionImplTest} for detailed behavior. | ||
| */ | ||
| public static Object collectElements(Object... args) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: Can we make this typing any more specific than
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot since input/output could be any type. |
||
| List<Object> elements = collectElementsToList(args); | ||
|
|
||
| if (elements.isEmpty()) { | ||
| return null; | ||
| } else if (elements.size() == 1) { | ||
| // return the element in case of single element | ||
| return elements.get(0); | ||
| } else { | ||
| return elements; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Collect non-null elements from `args`. If an item is a list, it will collect non-null elements. | ||
| */ | ||
| public static List<Object> collectElements(Object... args) { | ||
| public static List<Object> collectElementsToList(Object... args) { | ||
| List<Object> elements = new ArrayList<>(); | ||
|
|
||
| for (Object arg : args) { | ||
|
|
@@ -28,7 +46,7 @@ public static List<Object> collectElements(Object... args) { | |
| } | ||
| } | ||
|
|
||
| return elements.isEmpty() ? null : elements; | ||
| return elements; | ||
| } | ||
|
|
||
| private static void addListElements(List<?> list, List<Object> elements) { | ||
|
|
||
65 changes: 65 additions & 0 deletions
65
...rc/main/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImpl.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.expression.function.CollectionUDF; | ||
|
|
||
| import java.util.List; | ||
| import org.apache.calcite.adapter.enumerable.NotNullImplementor; | ||
| import org.apache.calcite.adapter.enumerable.NullPolicy; | ||
| import org.apache.calcite.adapter.enumerable.RexToLixTranslator; | ||
| import org.apache.calcite.linq4j.tree.Expression; | ||
| import org.apache.calcite.linq4j.tree.Expressions; | ||
| import org.apache.calcite.linq4j.tree.Types; | ||
| import org.apache.calcite.rel.type.RelDataTypeFactory; | ||
| import org.apache.calcite.rex.RexCall; | ||
| import org.apache.calcite.sql.type.SqlReturnTypeInference; | ||
| import org.apache.calcite.sql.type.SqlTypeName; | ||
| import org.opensearch.sql.expression.function.ImplementorUDF; | ||
| import org.opensearch.sql.expression.function.UDFOperandMetadata; | ||
|
|
||
| /** | ||
| * Internal append function that appends all elements from arguments to create an array. Returns | ||
| * null if there is no element. Returns the scalar value if there is single element. Otherwise, | ||
| * returns a list containing all the elements from inputs. | ||
| */ | ||
| public class AppendFunctionImpl extends ImplementorUDF { | ||
|
|
||
| public AppendFunctionImpl() { | ||
| super(new AppendImplementor(), NullPolicy.ALL); | ||
| } | ||
|
|
||
| @Override | ||
| public SqlReturnTypeInference getReturnTypeInference() { | ||
| return sqlOperatorBinding -> { | ||
| RelDataTypeFactory typeFactory = sqlOperatorBinding.getTypeFactory(); | ||
|
|
||
| if (sqlOperatorBinding.getOperandCount() == 0) { | ||
| return typeFactory.createSqlType(SqlTypeName.NULL); | ||
| } | ||
|
|
||
| // Return type is ANY as it could return scalar value (in case of single item) or array | ||
| return typeFactory.createSqlType(SqlTypeName.ANY); | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public UDFOperandMetadata getOperandMetadata() { | ||
| return null; | ||
| } | ||
|
|
||
| public static class AppendImplementor implements NotNullImplementor { | ||
| @Override | ||
| public Expression implement( | ||
| RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) { | ||
| return Expressions.call( | ||
| Types.lookupMethod(AppendFunctionImpl.class, "append", Object[].class), | ||
| Expressions.newArrayInit(Object.class, translatedOperands)); | ||
| } | ||
| } | ||
|
|
||
| public static Object append(Object... args) { | ||
| return AppendCore.collectElements(args); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
81 changes: 81 additions & 0 deletions
81
...est/java/org/opensearch/sql/expression/function/CollectionUDF/AppendFunctionImplTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.expression.function.CollectionUDF; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class AppendFunctionImplTest { | ||
|
|
||
| @Test | ||
| public void testAppendWithNoArguments() { | ||
| Object result = AppendFunctionImpl.append(); | ||
| assertNull(result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithSingleElement() { | ||
| Object result = AppendFunctionImpl.append(42); | ||
| assertEquals(42, result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithMultipleElements() { | ||
| Object result = AppendFunctionImpl.append(1, 2, 3); | ||
| assertEquals(Arrays.asList(1, 2, 3), result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithNullValues() { | ||
| Object result = AppendFunctionImpl.append(null, 1, null); | ||
| assertEquals(1, result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithAllNulls() { | ||
| Object result = AppendFunctionImpl.append(null, null); | ||
| assertNull(result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithArrayFlattening() { | ||
| List<Integer> array1 = Arrays.asList(1, 2); | ||
| List<Integer> array2 = Arrays.asList(3, 4); | ||
| Object result = AppendFunctionImpl.append(array1, array2); | ||
| assertEquals(Arrays.asList(1, 2, 3, 4), result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithMixedTypes() { | ||
| List<Integer> array = Arrays.asList(1, 2); | ||
| Object result = AppendFunctionImpl.append(array, 3, "hello"); | ||
| assertEquals(Arrays.asList(1, 2, 3, "hello"), result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithArrayAndNulls() { | ||
| List<Integer> array = Arrays.asList(1, 2); | ||
| Object result = AppendFunctionImpl.append(null, array, null, 3); | ||
| assertEquals(Arrays.asList(1, 2, 3), result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithSingleNull() { | ||
| Object result = AppendFunctionImpl.append((Object) null); | ||
| assertNull(result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendWithEmptyArray() { | ||
| List<Object> emptyArray = Arrays.asList(); | ||
| Object result = AppendFunctionImpl.append(emptyArray, 1); | ||
| assertEquals(1, result); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in standup this morning that I think we're making this more complex by working directly on the
Listof fields.I'd like to consider putting the bulk of this logic in a data class that can start supplying a higher-level interface for most field operations. Can incrementally move more commands to use it over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is very special logic for
spathto append values to static fields and remove appended ones from dynamic fields. I don't think it is something reusable for other use case, and should be coded inspathimplementation rather than abstraction layer.spathcommand is special since it generate new dynamic fields while others only consume.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If spath command is special enough for this, I imagine other commands would have their own special cases later. It would still be worth finding an abstraction here in my view. To me it seems like this is one of the main operations of adding dynamic fields to that structure & would eventually be reusable as more dynamic commands are visited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected commands which would add dynamic fields are:
multikvandtimechart(There are some more with lower priorities: chart, extract, kv, xpath, xmlkv, transpose, xyseries, pivot)multikvwould simply overwrite existing fields.timechartwould also overwrite existing fields (it is a bit different since it would rather make new frame rather than updating existing frame)There might be a chance we could reuse current logic in the future, but I think we should generalize when we implement that. I don't have very good idea to generalize some part right now. Let me know if you already have clear picture which part should be generalized.