-
Notifications
You must be signed in to change notification settings - Fork 141
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
Enhance OS value parsing #3002
base: main
Are you sure you want to change the base?
Enhance OS value parsing #3002
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.sql.sql; | ||
|
||
import static org.opensearch.sql.legacy.SQLIntegTestCase.Index.DATA_TYPE_NONNUMERIC; | ||
import static org.opensearch.sql.legacy.SQLIntegTestCase.Index.DATA_TYPE_NUMERIC; | ||
import static org.opensearch.sql.util.MatcherUtils.rows; | ||
import static org.opensearch.sql.util.MatcherUtils.schema; | ||
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; | ||
import static org.opensearch.sql.util.MatcherUtils.verifySchema; | ||
|
||
import java.io.IOException; | ||
import org.json.JSONObject; | ||
import org.junit.Test; | ||
import org.opensearch.sql.legacy.SQLIntegTestCase; | ||
|
||
public class DataTypeIT extends SQLIntegTestCase { | ||
@Override | ||
public void init() throws IOException { | ||
loadIndex(DATA_TYPE_NUMERIC); | ||
loadIndex(DATA_TYPE_NONNUMERIC); | ||
} | ||
|
||
@Test | ||
public void testReadingData() throws IOException { | ||
String query = | ||
String.format( | ||
"SELECT" | ||
+ " long_number,integer_number,short_number,byte_number,double_number,float_number,half_float_number,scaled_float_number" | ||
+ " FROM %s LIMIT 5", | ||
Index.DATA_TYPE_NUMERIC.getName()); | ||
JSONObject result = executeJdbcRequest(query); | ||
verifySchema( | ||
result, | ||
schema("long_number", "long"), | ||
schema("integer_number", "integer"), | ||
schema("short_number", "short"), | ||
schema("byte_number", "byte"), | ||
schema("float_number", "float"), | ||
schema("double_number", "double"), | ||
schema("half_float_number", "float"), | ||
schema("scaled_float_number", "double")); | ||
verifyDataRows( | ||
result, | ||
rows(1, 2, 3, 4, 5.1, 6.2, 7.3, 8.4), | ||
rows(1, 2, 3, 4, 5.1, 6.2, 7.3, 8.4), | ||
rows(1, 2, 3, 4, 5.1, 6.2, 7.3, 8.4)); | ||
|
||
query = | ||
String.format( | ||
"SELECT boolean_value,keyword_value FROM %s LIMIT 5", | ||
Index.DATA_TYPE_NONNUMERIC.getName()); | ||
result = executeJdbcRequest(query); | ||
verifySchema(result, schema("boolean_value", "boolean"), schema("keyword_value", "keyword")); | ||
verifyDataRows(result, rows(true, "123"), rows(true, "123"), rows(true, "123")); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why add limit 1? 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. same as above |
||
TEST_INDEX_DATATYPE_NUMERIC)); | ||
verifyDataRows( | ||
response, rows("DOUBLE", "LONG", "INTEGER", "BYTE", "SHORT", "FLOAT", "FLOAT", "DOUBLE")); | ||
|
@@ -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;", | ||
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. ditto |
||
TEST_INDEX_DATATYPE_NONNUMERIC)); | ||
verifyDataRows( | ||
response, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,6 @@ | ||
{"index":{"_id":"1"}} | ||
{"boolean_value": true, "keyword_value": "keyword", "text_value": "text", "binary_value": "U29tZSBiaW5hcnkgYmxvYg==", "date_value": "2020-10-13 13:00:00", "date_nanos_value": "2019-03-23T21:34:46.123456789-04:00", "ip_value": "127.0.0.1", "object_value": {"first": "Dale", "last": "Dale"}, "nested_value": [{"first" : "John", "last" : "Smith"}, {"first" : "Alice", "last" : "White"}], "geo_point_value": { "lat": 40.71, "lon": 74.00 }} | ||
{"boolean_value": true, "keyword_value": "123", "text_value": "text", "binary_value": "U29tZSBiaW5hcnkgYmxvYg==", "date_value": "2020-10-13 13:00:00", "date_nanos_value": "2019-03-23T21:34:46.123456789-04:00", "ip_value": "127.0.0.1", "object_value": {"first": "Dale", "last": "Dale"}, "nested_value": [{"first" : "John", "last" : "Smith"}, {"first" : "Alice", "last" : "White"}], "geo_point_value": { "lat": 40.71, "lon": 74.00 }} | ||
{"index":{"_id":"2"}} | ||
{"boolean_value": "true", "keyword_value": 123, "text_value": "text", "binary_value": "U29tZSBiaW5hcnkgYmxvYg==", "date_value": "2020-10-13 13:00:00", "date_nanos_value": "2019-03-23T21:34:46.123456789-04:00", "ip_value": "127.0.0.1", "object_value": {"first": "Dale", "last": "Dale"}, "nested_value": [{"first" : "John", "last" : "Smith"}, {"first" : "Alice", "last" : "White"}], "geo_point_value": { "lat": 40.71, "lon": 74.00 }} | ||
{"index":{"_id":"3"}} | ||
{"boolean_value": ["true"], "keyword_value": [123], "text_value": "text", "binary_value": "U29tZSBiaW5hcnkgYmxvYg==", "date_value": "2020-10-13 13:00:00", "date_nanos_value": "2019-03-23T21:34:46.123456789-04:00", "ip_value": "127.0.0.1", "object_value": {"first": "Dale", "last": "Dale"}, "nested_value": [{"first" : "John", "last" : "Smith"}, {"first" : "Alice", "last" : "White"}], "geo_point_value": { "lat": 40.71, "lon": 74.00 }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,6 @@ | ||
{"index":{"_id":"1"}} | ||
{"long_number": 1, "integer_number": 2, "short_number": 3, "byte_number": 4, "double_number": 5.1, "float_number": 6.2, "half_float_number": 7.3, "scaled_float_number": 8.4} | ||
{"index":{"_id":"2"}} | ||
{"long_number": "1", "integer_number": "2", "short_number": "3", "byte_number": "4", "double_number": "5.1", "float_number": "6.2", "half_float_number": "7.3", "scaled_float_number": "8.4"} | ||
{"index":{"_id":"3"}} | ||
{"long_number": ["1"], "integer_number": ["2"], "short_number": ["3"], "byte_number": ["4"], "double_number": ["5.1"], "float_number": ["6.2"], "half_float_number": ["7.3"], "scaled_float_number": ["8.4"]} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,10 +60,8 @@ | |
import org.opensearch.sql.data.model.ExprValue; | ||
import org.opensearch.sql.data.type.ExprCoreType; | ||
import org.opensearch.sql.data.type.ExprType; | ||
import org.opensearch.sql.opensearch.data.type.OpenSearchBinaryType; | ||
import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; | ||
import org.opensearch.sql.opensearch.data.type.OpenSearchDateType; | ||
import org.opensearch.sql.opensearch.data.type.OpenSearchIpType; | ||
import org.opensearch.sql.opensearch.data.utils.Content; | ||
import org.opensearch.sql.opensearch.data.utils.ObjectContent; | ||
import org.opensearch.sql.opensearch.data.utils.OpenSearchJsonContent; | ||
|
@@ -346,9 +344,8 @@ private ExprValue parseArray( | |
if (content.objectValue() instanceof ObjectNode) { | ||
result.add(parseStruct(content, prefix, supportArrays)); | ||
// non-object type arrays are only supported when parsing inner_hits of OS response. | ||
} else if (!(type instanceof OpenSearchDataType | ||
&& ((OpenSearchDataType) type).getExprType().equals(ARRAY)) | ||
&& !supportArrays) { | ||
} else if (((OpenSearchDataType) type).getExprType().equals(ARRAY) == false | ||
&& supportArrays == false) { | ||
Comment on lines
+347
to
+348
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. what is the change? could you elaborate more? 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. I think the type must be OpenSearchDataType after my change, please correct me if I'm wrong 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. I think it is not required, the change improvment the json parse behaviour to support read int value from [int, string] value. |
||
return parseInnerArrayValue(content.array().next(), prefix, type, supportArrays); | ||
} else { | ||
content | ||
|
@@ -415,10 +412,10 @@ private ExprValue parseGeoPoint(Content content, boolean supportArrays) { | |
*/ | ||
private ExprValue parseInnerArrayValue( | ||
Content content, String prefix, ExprType type, boolean supportArrays) { | ||
if (type instanceof OpenSearchIpType | ||
|| type instanceof OpenSearchBinaryType | ||
|| type instanceof OpenSearchDateType) { | ||
return parse(content, prefix, Optional.of(type), supportArrays); | ||
if (content.isArray()) { | ||
return parseArray(content, prefix, type, supportArrays); | ||
} else if (typeActionMap.containsKey(type)) { | ||
return typeActionMap.get(type).apply(content, type); | ||
} else if (content.isString()) { | ||
return parse(content, prefix, Optional.of(OpenSearchDataType.of(STRING)), supportArrays); | ||
} else if (content.isLong()) { | ||
|
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.
what is current result if remove head 1? is it breaking change?
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 add more rows in
TEST_INDEX_DATATYPE_NONNUMERIC
index, head 1 to avoid change the test.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.
the return result order is not guaranteed, why not add another test dataset?