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 23:01:40 UTC
[GitHub] [pinot] agavra opened a new pull request, #9818: [multistage] m0ar tests! (binary, boolean, char, time)
agavra opened a new pull request, #9818:
URL: https://github.com/apache/pinot/pull/9818
see description. Also:
- fixed a bug in `DataBlockBuilder` where it's possible to get a `byte[]` instead of `ByteArray` from transferable block - (the Segment builder uses `PintoDataType#convert`, which for `PinotDataType.BYTES` returns a `byte[]` and not a `ByteArray`). Not sure if this is a bug in Pinot v1, but I figured it's safer to just fix `DataBlockBuilder` to account for both cases.
- cleaned up some error messages
- added necessary comparators for testing
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025881809
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -268,9 +274,18 @@ private static List<String> toH2FieldNamesAndTypes(org.apache.pinot.spi.data.Sch
case DOUBLE:
fieldType = "double";
break;
+ case BOOLEAN:
+ fieldType = "BOOLEAN";
+ break;
case BIG_DECIMAL:
fieldType = "NUMERIC";
break;
+ case BYTES:
+ fieldType = "BYTEA";
Review Comment:
no, postgres and SQL standard calls this `BYTEA` (I think it stands for byte array)
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025756060
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -268,9 +274,18 @@ private static List<String> toH2FieldNamesAndTypes(org.apache.pinot.spi.data.Sch
case DOUBLE:
fieldType = "double";
break;
+ case BOOLEAN:
+ fieldType = "BOOLEAN";
+ break;
case BIG_DECIMAL:
fieldType = "NUMERIC";
break;
+ case BYTES:
+ fieldType = "BYTEA";
Review Comment:
typo? should this be `"BYTES"`?
##########
pinot-query-runtime/src/test/resources/queries/BooleanLogic.json:
##########
@@ -0,0 +1,106 @@
+{
+ "boolean_type": {
+ "tables": {
+ "bools": {
+ "schema": [{"name": "b", "type": "BOOLEAN"}],
+ "inputs": [[true], [false]]
+ }
+ },
+ "queries": [
+ {
+ "description": "should support booleans",
+ "sql": "select b FROM {bools}"
+ },
+ {
+ "description": "should support boolean literals",
+ "sql": "select b = true, b = false FROM {bools}"
+ }
+ ]
+ },
+ "boolean_implicit_casting": {
+ "tables": {
+ "tbl": {
+ "schema": [
+ {"name": "b", "type": "BOOLEAN"},
+ {"name": "i", "type": "INT"},
+ {"name": "d", "type": "DOUBLE"},
+ {"name": "s", "type": "STRING"},
+ {"name": "n", "type": "BIG_DECIMAL"},
+ {"name": "t", "type": "TIMESTAMP"},
+ {"name": "by", "type": "BYTES"}
+ ],
+ "inputs": [
+ [true, 1, 1.2, "foo", "1.234", "2022-01-02 03:45:00", "DEADBEEF"],
+ [true, 0, 0.0, "", "0.0", "2022-01-02 03:45:00", "00"],
+ [false, 1, 1.2, "foo", "1.234", "2022-01-02 03:45:00", "DEADBEEF"],
+ [false, 0, 0.0, "", "0.0", "2022-01-02 03:45:00", "00"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "note": "to be postgres compatible, this should throw an exception - but various other systems (like H2 and MySQL) support this",
+ "description": "check implicit cast between boolean and int",
+ "sql": "select b = i FROM {tbl}"
Review Comment:
can we add some function mix?
- boolean logic cascade: `(b = i) = true`
- boolean logic combine `(b = i) AND/OR (b = n)`
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -129,6 +130,9 @@ public QueryPlan planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions)
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
return toDispatchablePlan(relRoot, plannerContext);
+ } catch (CalciteContextException e) {
+ throw new RuntimeException("Error composing query plan for '" + sqlQuery
+ + "': " + e.getMessage() + "'", e);
Review Comment:
could we add a better message? what's the benefit for adding this extra catch clause?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -63,7 +63,11 @@ public PinotCatalog(TableCache tableCache) {
@Override
public Table getTable(String name) {
String tableName = TableNameBuilder.extractRawTableName(name);
- return new PinotTable(_tableCache.getSchema(tableName));
+ org.apache.pinot.spi.data.Schema schema = _tableCache.getSchema(tableName);
+ if (schema == null) {
+ throw new IllegalArgumentException("Could not find schema for table: " + tableName);
+ }
Review Comment:
nit: by the time it reaches here. it shouldn't be couldn't find the schema for a table but more of an internal error as PinotCatalog is inconsistent of the table it said it contains vs. the schema it can pull out from.
but I don't know what's a better error message to indicate this info.
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025881018
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -129,6 +130,9 @@ public QueryPlan planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions)
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
return toDispatchablePlan(relRoot, plannerContext);
+ } catch (CalciteContextException e) {
+ throw new RuntimeException("Error composing query plan for '" + sqlQuery
+ + "': " + e.getMessage() + "'", e);
Review Comment:
This allows us to bubble up the error from the underlying message. e.g. instead of the user seeing `"Error composing query plan for : 'some sql'"` they'll see more details in the case of a `CalciteContextException`, which will give info like `Cannot use operator '=' to compare <STRING> and <INTEGER>`. I didn't want to bubble up any random exception because some might have really unehlpeful error messages, thats' why I separate out the clauses.
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025901955
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -129,6 +130,9 @@ public QueryPlan planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions)
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
return toDispatchablePlan(relRoot, plannerContext);
+ } catch (CalciteContextException e) {
+ throw new RuntimeException("Error composing query plan for '" + sqlQuery
+ + "': " + e.getMessage() + "'", e);
Review Comment:
hmm. I am not 100% sure i understand. `e.getMessage()` is also included in the `e` part which is also part of the other catch clause branch below correct?
or the other way to ask is why can't we add e.getMessage() to both branches of catch exception?
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025845163
##########
pinot-query-runtime/src/test/resources/queries/TimeTypes.json:
##########
@@ -0,0 +1,72 @@
+{
+ "ts_without_tz": {
+ "tables": {
+ "ts": {
+ "schema": [
+ {"name": "data", "type": "TIMESTAMP"}
+ ],
+ "inputs": [
+ ["1999-01-08 04:05:06"],
+ ["1999-01-08 22:05:46"],
+ ["1999-01-08 04:05:06.001"],
+ ["5760-01-01 04:05:06"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.5.1.3",
+ "description": "should support timestamps with various values",
Review Comment:
I thought we don't support time and byte types yet since there are a lot of functions we don't support yet.
@walterddr We were saying earlier we only support primitive types + string for v2 engine now. Does this narrative change?
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9818:
URL: https://github.com/apache/pinot/pull/9818#issuecomment-1319283633
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9818?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 [#9818](https://codecov.io/gh/apache/pinot/pull/9818?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8b2af1a) into [master](https://codecov.io/gh/apache/pinot/commit/73e6129941d5099e83a0ceca1eae388a38f6b8c4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (73e6129) will **increase** coverage by `38.76%`.
> The diff coverage is `60.00%`.
```diff
@@ Coverage Diff @@
## master #9818 +/- ##
=============================================
+ Coverage 24.55% 63.32% +38.76%
- Complexity 53 4776 +4723
=============================================
Files 1952 1952
Lines 104676 104684 +8
Branches 15856 15858 +2
=============================================
+ Hits 25700 66286 +40586
+ Misses 76347 33530 -42817
- Partials 2629 4868 +2239
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.35% <0.00%> (?)` | |
| integration2 | `24.57% <0.00%> (+0.02%)` | :arrow_up: |
| unittests1 | `67.89% <60.00%> (?)` | |
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/9818?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/core/common/datablock/DataBlockBuilder.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL0RhdGFCbG9ja0J1aWxkZXIuamF2YQ==) | `74.23% <33.33%> (+74.23%)` | :arrow_up: |
| [...a/org/apache/pinot/query/catalog/PinotCatalog.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvY2F0YWxvZy9QaW5vdENhdGFsb2cuamF2YQ==) | `47.36% <50.00%> (+47.36%)` | :arrow_up: |
| [.../java/org/apache/pinot/query/QueryEnvironment.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvUXVlcnlFbnZpcm9ubWVudC5qYXZh) | `84.84% <100.00%> (+84.84%)` | :arrow_up: |
| [.../helix/core/minion/MinionInstancesCleanupTask.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9NaW5pb25JbnN0YW5jZXNDbGVhbnVwVGFzay5qYXZh) | `54.54% <0.00%> (-9.10%)` | :arrow_down: |
| [...tream/kafka20/server/KafkaDataServerStartable.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWthZmthLTIuMC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL3N0cmVhbS9rYWZrYTIwL3NlcnZlci9LYWZrYURhdGFTZXJ2ZXJTdGFydGFibGUuamF2YQ==) | `72.91% <0.00%> (-6.25%)` | :arrow_down: |
| [...pache/pinot/core/query/utils/idset/EmptyIdSet.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS91dGlscy9pZHNldC9FbXB0eUlkU2V0LmphdmE=) | `25.00% <0.00%> (ø)` | |
| [...ot/common/function/scalar/ComparisonFunctions.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0NvbXBhcmlzb25GdW5jdGlvbnMuamF2YQ==) | `42.85% <0.00%> (ø)` | |
| [...anager/realtime/SegmentBuildTimeLeaseExtender.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudEJ1aWxkVGltZUxlYXNlRXh0ZW5kZXIuamF2YQ==) | `63.23% <0.00%> (ø)` | |
| [...g/apache/pinot/common/datatable/BaseDataTable.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YXRhYmxlL0Jhc2VEYXRhVGFibGUuamF2YQ==) | `78.94% <0.00%> (+0.75%)` | :arrow_up: |
| [...e/pinot/common/function/TransformFunctionType.java](https://codecov.io/gh/apache/pinot/pull/9818/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25UeXBlLmphdmE=) | `100.00% <0.00%> (+0.94%)` | :arrow_up: |
| ... and [1240 more](https://codecov.io/gh/apache/pinot/pull/9818/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] agavra commented on a diff in pull request #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025881593
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -63,7 +63,11 @@ public PinotCatalog(TableCache tableCache) {
@Override
public Table getTable(String name) {
String tableName = TableNameBuilder.extractRawTableName(name);
- return new PinotTable(_tableCache.getSchema(tableName));
+ org.apache.pinot.spi.data.Schema schema = _tableCache.getSchema(tableName);
+ if (schema == null) {
+ throw new IllegalArgumentException("Could not find schema for table: " + tableName);
+ }
Review Comment:
I updated the error message, hopefully it's a bit better.
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025889783
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -268,9 +274,18 @@ private static List<String> toH2FieldNamesAndTypes(org.apache.pinot.spi.data.Sch
case DOUBLE:
fieldType = "double";
break;
+ case BOOLEAN:
+ fieldType = "BOOLEAN";
+ break;
case BIG_DECIMAL:
fieldType = "NUMERIC";
break;
+ case BYTES:
+ fieldType = "BYTEA";
Review Comment:
ah..I see. Thanks !!!
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9818:
URL: https://github.com/apache/pinot/pull/9818
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025841616
##########
pinot-query-runtime/src/test/resources/queries/BooleanLogic.json:
##########
@@ -0,0 +1,106 @@
+{
+ "boolean_type": {
Review Comment:
Can we add Select true from tbl, select xx from tbl where true (where col = true)
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025842606
##########
pinot-query-runtime/src/test/resources/queries/CharacterTypes.json:
##########
@@ -0,0 +1,31 @@
+{
+ "fixed_char": {
+ "ignored": true,
+ "comment": "we don't support fixed length string types",
+ "psql": "8.3"
+ },
+ "varchar": {
+ "tables": {
+ "varchar": {
+ "schema": [
+ {"name": "str", "type": "STRING"}
+ ],
+ "inputs": [
+ ["foo"],
+ ["value with spaces"],
+ ["Οὐχὶ (greek)"],
+ ["แสน (thai)"],
+ ["верстке (russian)"],
+ ["∀x∈ℝ (mathematics)"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "psql": "8.3",
+ "description": "test different UTF8 string values",
+ "sql": "SELECT * FROM {varchar}"
Review Comment:
Add select "string" from tbl?
--
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 #9818: [multistage] m0ar tests! (binary, boolean, char, time)
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1026636713
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -129,6 +130,9 @@ public QueryPlan planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions)
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
return toDispatchablePlan(relRoot, plannerContext);
+ } catch (CalciteContextException e) {
+ throw new RuntimeException("Error composing query plan for '" + sqlQuery
+ + "': " + e.getMessage() + "'", e);
Review Comment:
the problem is that what's shown to the user isn't the entire stack trace, that part gets swallowed upstream somewhere (and that makes sense, that would make for a bad user experience to see the entire stack trace and causedBy). All they see is what's in the error message.
> or the other way to ask is why can't we add e.getMessage() to both branches of catch exception?
that's what I was saying earlier. I don't want to add the cause if the exception makes no sense (like a `NullPointerException`) - the user shouldn't see that kind of message as it really isn't helpful.
--
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