Support spath command to extract all fields #4822
Conversation
ykmr1224
added
PPL
ykmr1224
changed the title
ykmr1224
force-pushed
the
dynamic-spath-append
branch
3 times, most recently
from
09ff0ca to
425da58
Compare
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
ykmr1224
force-pushed
the
dynamic-spath-append
branch
from
eaa3aed to
28cbf06
Compare
ykmr1224
requested review from
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48,
seankao-az and
vamsimanohar
as code owners
ykmr1224
requested review from
GumpacG,
LantaoJin,
RyanL1997,
noCharger,
qianheng-aws and
yuancu
as code owners
| } | ||
|
|
||
| @Test | ||
| @Ignore("Join does not work now") |
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Outdated
Show resolved
Hide resolved
| // 2. Merge dynamic fields with static fields. | ||
| // static_field1 = APPEND(static_field1, _MAP[static_field1]), static_attr2 = ... | ||
| List<String> staticFieldNames = context.fieldBuilder.getStaticFieldNames(); | ||
| List<RexNode> fields = new ArrayList<>(); |
There was a problem hiding this comment.
I mentioned in standup this morning that I think we're making this more complex by working directly on the List of 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.
This one is very special logic for spath to 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 in spath implementation rather than abstraction layer.
spath command is special since it generate new dynamic fields while others only consume.
There was a problem hiding this comment.
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.
Expected commands which would add dynamic fields are: multikv and timechart (There are some more with lower priorities: chart, extract, kv, xpath, xmlkv, transpose, xyseries, pivot)
multikv would simply overwrite existing fields.
timechart would 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.
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/MapAppendFunctionIT.java
Outdated
Show resolved
Hide resolved
| * of the list. See {@ref MVAppendFunctionImplTest} for detailed behavior. | ||
| * of the list. See {@ref AppendFunctionImplTest} for detailed behavior. | ||
| */ | ||
| public static Object collectElements(Object... args) { |
There was a problem hiding this comment.
thought: Can we make this typing any more specific than Object?
There was a problem hiding this comment.
We cannot since input/output could be any type.
Signed-off-by: Tomoyuki Morita <[email protected]>
|
This PR is stalled because it has been open for 2 weeks with no activity. |
2 participants
This PR is for feature branch
feature/permissiveDescription
_MAP(dynamic fields) as we cannot know the contents at planning time.ANYtype because append could happen depending on the JSON content.APPENDto keep existing field scalar, and make single item result scalarMVAPPENDwhich returns array even for single value result, but realized it makes other commands require conversion from array to scalar. I considered automatic type coercion from array to scalar, but it does not align with Splunk behavior where multivalue with single item would be automatically converted as scalar value.Related Issues
Permissive mode RFC: #4349
Dynamic fields RFC: #4433
Check List
--signoffor-s.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.