Enhance OS value parsing#3002
Conversation
Signed-off-by: panguixin <panguixin@bytedance.com>
bugmakerrrrrr
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48,
rupal-bq,
seankao-az,
vamsimanohar and
ykmr1224
as code owners
penghuo
left a comment
penghuo
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
One questions, does the change in OpenSearchExprValueFactory.java related to the issue?
opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java
Show resolved
Hide resolved
| } else if (((OpenSearchDataType) type).getExprType().equals(ARRAY) == false | ||
| && supportArrays == false) { |
There was a problem hiding this comment.
what is the change? could you elaborate more?
There was a problem hiding this comment.
I think the type must be OpenSearchDataType after my change, please correct me if I'm wrong
There was a problem hiding this comment.
I think it is not required, the change improvment the json parse behaviour to support read int value from [int, string] value.
| "SELECT typeof(double_number),typeof(long_number), typeof(integer_number)," | ||
| + " typeof(byte_number), typeof(short_number),typeof(float_number)," | ||
| + " typeof(half_float_number), typeof(scaled_float_number) from %s;", | ||
| + " typeof(half_float_number), typeof(scaled_float_number) from %s limit 1;", |
There was a problem hiding this comment.
same as above
| // ExpressionAnalyzer::isTypeNotSupported | ||
| // + ", typeof(nested_value)" | ||
| + " from %s;", | ||
| + " from %s limit 1;", |
| executeQuery( | ||
| String.format( | ||
| "source=%s | eval `%s` = %s | fields `%s`", | ||
| "source=%s | head 1 | eval `%s` = %s | fields `%s`", |
There was a problem hiding this comment.
what is current result if remove head 1? is it breaking change?
There was a problem hiding this comment.
I add more rows in TEST_INDEX_DATATYPE_NONNUMERIC index, head 1 to avoid change the test.
There was a problem hiding this comment.
the return result order is not guaranteed, why not add another test dataset?
Yes, we need to parse the value in array according to the type of field, not the type of content |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
Hi @bugmakerrrrrr , do we still need this change? |
3 participants
Description
#3001
Related Issues
Resolves #3001
Check List
--signoff.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.