Enhance OS value parsing #3002
Enhance OS value parsing #3002
Conversation
Signed-off-by: panguixin <[email protected]>
bugmakerrrrrr
requested review from
ps48,
kavithacm,
derek-ho,
joshuali925,
dai-chen,
YANG-DB,
rupal-bq,
mengweieric,
vamsi-amazon,
Swiddis,
penghuo,
seankao-az,
MaxKsyunz,
Yury-Fridlyand,
anirudha,
forestmvey,
acarbonetto,
GumpacG,
ykmr1224 and
LantaoJin
as code owners
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.
| @@ -46,7 +46,7 @@ public void typeof_opensearch_types() { | |||
| String.format( | |||
| "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
| @@ -61,7 +61,7 @@ public void typeof_opensearch_types() { | |||
| // TODO activate this test once `ARRAY` type supported, see | |||
| // ExpressionAnalyzer::isTypeNotSupported | |||
| // + ", typeof(nested_value)" | |||
| + " from %s;", | |||
| + " from %s limit 1;", | |||
| @@ -401,7 +401,7 @@ public void testCompare() throws IOException { | |||
| var result = | |||
| 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 |
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.