You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/16 00:18:44 UTC
[GitHub] [pinot] agavra opened a new pull request, #9806: [multistage] add big_decimal support and numeric type tests
agavra opened a new pull request, #9806:
URL: https://github.com/apache/pinot/pull/9806
1. add support for `BIG_DECIMAL` in the multistage engine
2. add some tests for numeric types - most of the interesting tests for numeric types will be done per-operator (+, -, %, etc...) this just covers that we support the types at all.
3. make the test framework a bit more verbose with error messages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023391132
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
Actually, I think the code I read before is for literal type. maybe it is working for the sql type. Let me double check and get back to you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024170893
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -292,6 +314,8 @@ public static class Query {
public String _description;
@JsonProperty("outputs")
public List<List<Object>> _outputs = Collections.emptyList();
+ @JsonProperty("expect")
+ public String _expect;
Review Comment:
nit: `expected_exception`?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -131,6 +137,12 @@ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]> expect
return ((String) l).compareTo((String) r);
} else if (l instanceof Boolean) {
return ((Boolean) l).compareTo((Boolean) r);
+ } else if (l instanceof BigDecimal) {
+ if (r instanceof BigDecimal) {
+ return ((BigDecimal) l).compareTo((BigDecimal) r);
+ } else {
+ return ((BigDecimal) l).compareTo(new BigDecimal((String) r));
Review Comment:
is this b/c the return type of Pinot is not BigDecimal or b/c H2 is not returning BigDecimal?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -151,6 +163,10 @@ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]> expect
Object[] resultRow = resultRows.get(i);
Object[] expectedRow = expectedRows.get(i);
for (int j = 0; j < resultRow.length; j++) {
+ if (j >= expectedRow.length) {
+ throw new AssertException(String.format("Unexpected row size mismatch. Expected: %s, Actual: %s",
+ Arrays.toString(expectedRow), Arrays.toString(resultRow)));
+ }
Review Comment:
good catch!!!
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
this refactoring seems convoluted. can't we simply do:
```suggestion
List<Object[]> resultRows;
if (except != null) {
try {
resultRows = queryRunner(SQL);
Assert.fail("Expected error with message '" + except + "'. But instead rows were returned: " + ...);
} catch (Exception e) {
Assert.assertTrue(...PatternMatch)
}
} else {
resultRows = queryRunner(sql);
}
```
IIRC Assert.fail throws AssertionError which is not an Exception so it won't be caught in the catch Exception clause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024266855
##########
pinot-query-runtime/src/test/resources/queries/NumericTypes.json:
##########
@@ -0,0 +1,117 @@
+{
+ "smallint": {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "not supported"
+ },
+ "integers": {
+ "comment": "we don't support BIGINT notation",
+ "tables": {
+ "ints": {
+ "schema": [
+ {"name": "int32", "type": "INT"},
+ {"name": "int64", "type": "LONG"}
+ ],
+ "inputs": [
+ [0, 0],
+ [123, 321],
+ [-2147483648, -9223372036854775808],
+ [2147483647, 9223372036854775807]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.1",
+ "description": "test selecting integer values",
+ "sql": "SELECT * FROM {ints}"
+ },
+ {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "we don't properly support overflow behavior because we return doubles",
+ "description": "overflow behavior",
+ "sql": "SELECT int32 + 1, int32 - 1, int64 + 1, int64 -1 FROM {ints}",
+ "expect": "*out of range*"
+ }
+ ]
+ },
+ "numeric": {
+ "comment": "ANSI SQL calls this type NUMERIC(precision, scale) but we call it BIG_DECIMAL",
+ "tables": {
+ "numeric": {
+ "schema": [
+ {"name": "big", "type": "BIG_DECIMAL"}
+ ],
+ "inputs": [
+ ["92233720368547758071"],
+ ["92233720368547758071.0000000001"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.2",
+ "TODO": "for some reason, when we select * with this test the timestamp column gets returned as well...",
Review Comment:
correct, that's why I'm not selecting `*` and instead selecting just the column :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024316851
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -151,6 +163,10 @@ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]> expect
Object[] resultRow = resultRows.get(i);
Object[] expectedRow = expectedRows.get(i);
for (int j = 0; j < resultRow.length; j++) {
+ if (j >= expectedRow.length) {
+ throw new AssertException(String.format("Unexpected row size mismatch. Expected: %s, Actual: %s",
+ Arrays.toString(expectedRow), Arrays.toString(resultRow)));
+ }
Review Comment:
no need to check on every `j` right?
```suggestion
Assert.assertTrue(resultRow.length == expectedRow.length, String.format("Unexpected row size mismatch. Expected: %s, Actual: %s",
Arrays.toString(expectedRow), Arrays.toString(resultRow)))
for (int j = 0; j < resultRow.length; j++) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024407637
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
I feel adding a new data type needs a lot more thorough testing. Say we make sure func calls work on this, filter/transform work on this etc.
It is better to say we don't support this type instead of we support this sometimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024467554
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
i originally though so too but this one is specific to null default handling. so I am ok with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024507294
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
also I think Rong was referring to `FieldSpec.java` where I added:
```
case BIG_DECIMAL:
return DEFAULT_DIMENSION_NULL_VALUE_OF_BIG_DECIMAL;
```
this change that you're commenting on was the TypeFactory that maps Calcite types to Pinot types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024478952
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
Hmm.. Why is this specific to null default handling?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023389693
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
If I remember the code correctly, this BIG_DECIMAL is not passed down correctly from broker to server in the thrift protocol. This is also one of the reason I feel we should do integration test at broker level.
Can you help verify this?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
Actually, I think the code I read before is for literal type. maybe it is working for the sql type. Let me double check and get back to you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024505708
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
I was planning on adding a lot more tests when I get to the mathematical operators. It's also not a new type, V1 supports it `BIG_DECIMAL` this was just necessary to wire v2 (calcite needs to know that we support it).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9806:
URL: https://github.com/apache/pinot/pull/9806
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023389693
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
If I remember the code correctly, this BIG_DECIMAL is not passed down correctly broker to server in the thrift protocol. This is also one of the reason I feel we should do integration test at broker level.
Can you help verify this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023421438
##########
pinot-query-runtime/src/test/resources/queries/NumericTypes.json:
##########
@@ -0,0 +1,117 @@
+{
+ "smallint": {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "not supported"
+ },
+ "integers": {
+ "comment": "we don't support BIGINT notation",
Review Comment:
we don't support BIGINT in schema definition. and we don't support LONG in SQL syntax. maybe we can make this a bit clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024505708
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
I was planning on adding a lot more tests when I get to the mathematical operators. It's also not a new type, V1 supports it `BIG_DECIMAL` this was just necessary to wire v2 with v1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024305534
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -131,6 +137,12 @@ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]> expect
return ((String) l).compareTo((String) r);
} else if (l instanceof Boolean) {
return ((Boolean) l).compareTo((Boolean) r);
+ } else if (l instanceof BigDecimal) {
+ if (r instanceof BigDecimal) {
+ return ((BigDecimal) l).compareTo((BigDecimal) r);
+ } else {
+ return ((BigDecimal) l).compareTo(new BigDecimal((String) r));
Review Comment:
JSON represents them as strings - so in the cases of specified `outputs` we need to be able to compare them to strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024561493
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
yeah, I assumed @61yao was talking also about the general supports, and not just the TypeFactory which is used exclusively for V2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024314334
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
I made it a bit cleaner, LMKWYT after I push 😉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023389693
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
If I remember the code correctly, this BIG_DECIMAL is not passed down correctly from broker to server in the thrift protocol. This is also one of the reason I feel we should do integration test at broker level.
Can you help verify this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1023422131
##########
pinot-query-runtime/src/test/resources/queries/NumericTypes.json:
##########
@@ -0,0 +1,117 @@
+{
+ "smallint": {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "not supported"
+ },
+ "integers": {
+ "comment": "we don't support BIGINT notation",
+ "tables": {
+ "ints": {
+ "schema": [
+ {"name": "int32", "type": "INT"},
+ {"name": "int64", "type": "LONG"}
+ ],
+ "inputs": [
+ [0, 0],
+ [123, 321],
+ [-2147483648, -9223372036854775808],
+ [2147483647, 9223372036854775807]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.1",
+ "description": "test selecting integer values",
+ "sql": "SELECT * FROM {ints}"
+ },
+ {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "we don't properly support overflow behavior because we return doubles",
+ "description": "overflow behavior",
+ "sql": "SELECT int32 + 1, int32 - 1, int64 + 1, int64 -1 FROM {ints}",
+ "expect": "*out of range*"
+ }
+ ]
+ },
+ "numeric": {
+ "comment": "ANSI SQL calls this type NUMERIC(precision, scale) but we call it BIG_DECIMAL",
+ "tables": {
+ "numeric": {
+ "schema": [
+ {"name": "big", "type": "BIG_DECIMAL"}
+ ],
+ "inputs": [
+ ["92233720368547758071"],
+ ["92233720368547758071.0000000001"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.2",
+ "TODO": "for some reason, when we select * with this test the timestamp column gets returned as well...",
Review Comment:
if this is true why is the test going through? shouldn't it failed b/c the number of columns don't match H2?
##########
pinot-query-runtime/src/test/resources/queries/NumericTypes.json:
##########
@@ -0,0 +1,117 @@
+{
+ "smallint": {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "not supported"
+ },
+ "integers": {
+ "comment": "we don't support BIGINT notation",
Review Comment:
we don't support BIGINT in schema definition. and we don't support LONG in SQL syntax.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024312968
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
why would you need to do this for H2? if you are asserting failure. you don't need H2 right?
(IIUC this is probably b/c the output can't be some of the queries list). for now we can do both but we will refactor that out later if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024320346
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9806:
URL: https://github.com/apache/pinot/pull/9806#issuecomment-1316121968
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9806?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9806](https://codecov.io/gh/apache/pinot/pull/9806?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2a04b70) into [master](https://codecov.io/gh/apache/pinot/commit/47c8f184365d6ffde870f94285747b9e009c7c15?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (47c8f18) will **increase** coverage by `8.20%`.
> The diff coverage is `86.86%`.
```diff
@@ Coverage Diff @@
## master #9806 +/- ##
============================================
+ Coverage 60.58% 68.79% +8.20%
+ Complexity 5281 4997 -284
============================================
Files 1949 1963 +14
Lines 104632 105031 +399
Branches 15847 15895 +48
============================================
+ Hits 63389 72252 +8863
+ Misses 36540 27725 -8815
- Partials 4703 5054 +351
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `24.42% <0.00%> (-0.03%)` | :arrow_down: |
| unittests1 | `67.85% <86.86%> (+0.04%)` | :arrow_up: |
| unittests2 | `15.77% <84.84%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9806?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/pinot/common/function/FunctionRegistry.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25SZWdpc3RyeS5qYXZh) | `86.84% <ø> (ø)` | |
| [...not/query/planner/logical/RelToStageConverter.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9sb2dpY2FsL1JlbFRvU3RhZ2VDb252ZXJ0ZXIuamF2YQ==) | `83.33% <ø> (+3.70%)` | :arrow_up: |
| [...che/pinot/query/planner/logical/RexExpression.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9sb2dpY2FsL1JleEV4cHJlc3Npb24uamF2YQ==) | `81.33% <ø> (+5.33%)` | :arrow_up: |
| [...ache/pinot/query/planner/logical/StagePlanner.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9sb2dpY2FsL1N0YWdlUGxhbm5lci5qYXZh) | `100.00% <ø> (ø)` | |
| [...e/pinot/query/runtime/operator/FilterOperator.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9GaWx0ZXJPcGVyYXRvci5qYXZh) | `76.00% <ø> (+16.00%)` | :arrow_up: |
| [...ery/runtime/operator/operands/FunctionOperand.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9vcGVyYW5kcy9GdW5jdGlvbk9wZXJhbmQuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...query/runtime/operator/MailboxReceiveOperator.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94UmVjZWl2ZU9wZXJhdG9yLmphdmE=) | `79.36% <50.00%> (-0.97%)` | :arrow_down: |
| [...query/runtime/operator/operands/FilterOperand.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9vcGVyYW5kcy9GaWx0ZXJPcGVyYW5kLmphdmE=) | `83.90% <50.00%> (+16.05%)` | :arrow_up: |
| [...ot/query/runtime/executor/RoundRobinScheduler.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9Sb3VuZFJvYmluU2NoZWR1bGVyLmphdmE=) | `87.50% <87.50%> (ø)` | |
| [...uery/runtime/executor/OpChainSchedulerService.java](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9PcENoYWluU2NoZWR1bGVyU2VydmljZS5qYXZh) | `95.12% <95.12%> (ø)` | |
| ... and [294 more](https://codecov.io/gh/apache/pinot/pull/9806/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024316851
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -151,6 +163,10 @@ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]> expect
Object[] resultRow = resultRows.get(i);
Object[] expectedRow = expectedRows.get(i);
for (int j = 0; j < resultRow.length; j++) {
+ if (j >= expectedRow.length) {
+ throw new AssertException(String.format("Unexpected row size mismatch. Expected: %s, Actual: %s",
+ Arrays.toString(expectedRow), Arrays.toString(resultRow)));
+ }
Review Comment:
```suggestion
Assert.assertTrue(resultRow.length == expectedRow.length, String.format("Unexpected row size mismatch. Expected: %s, Actual: %s",
Arrays.toString(expectedRow), Arrays.toString(resultRow)))
for (int j = 0; j < resultRow.length; j++) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024521035
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -70,6 +70,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return createSqlType(SqlTypeName.VARCHAR);
case BYTES:
return createSqlType(SqlTypeName.VARBINARY);
+ case BIG_DECIMAL:
Review Comment:
SG
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024278153
##########
pinot-query-runtime/src/test/resources/queries/NumericTypes.json:
##########
@@ -0,0 +1,117 @@
+{
+ "smallint": {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "not supported"
+ },
+ "integers": {
+ "comment": "we don't support BIGINT notation",
+ "tables": {
+ "ints": {
+ "schema": [
+ {"name": "int32", "type": "INT"},
+ {"name": "int64", "type": "LONG"}
+ ],
+ "inputs": [
+ [0, 0],
+ [123, 321],
+ [-2147483648, -9223372036854775808],
+ [2147483647, 9223372036854775807]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.1",
+ "description": "test selecting integer values",
+ "sql": "SELECT * FROM {ints}"
+ },
+ {
+ "psql": "8.1.1",
+ "ignored": true,
+ "comment": "we don't properly support overflow behavior because we return doubles",
+ "description": "overflow behavior",
+ "sql": "SELECT int32 + 1, int32 - 1, int64 + 1, int64 -1 FROM {ints}",
+ "expect": "*out of range*"
+ }
+ ]
+ },
+ "numeric": {
+ "comment": "ANSI SQL calls this type NUMERIC(precision, scale) but we call it BIG_DECIMAL",
+ "tables": {
+ "numeric": {
+ "schema": [
+ {"name": "big", "type": "BIG_DECIMAL"}
+ ],
+ "inputs": [
+ ["92233720368547758071"],
+ ["92233720368547758071.0000000001"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.1.2",
+ "TODO": "for some reason, when we select * with this test the timestamp column gets returned as well...",
Review Comment:
oh. I thought you meant select just the column will result the same as select *, my mistake
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9806: [multistage] add big_decimal support and numeric type tests
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024309759
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql, List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
> IIRC Assert.fail throws AssertionError which is not an Exception so it won't be caught in the catch Exception clause.
ah good call!
> this refactoring seems convoluted. can't we simply do:
I can, but then I need to do it twice (once for the h2 compat ones, ones for the output checkers)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org