You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/05/11 10:04:42 UTC
[GitHub] [druid] clintropolis opened a new pull request #11233: array handling improvements
clintropolis opened a new pull request #11233:
URL: https://github.com/apache/druid/pull/11233
### Description
TBD: _fix jdbc array handling, split handling for some array and multi value operators, split and add more tests_
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11233:
URL: https://github.com/apache/druid/pull/11233
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11233:
URL: https://github.com/apache/druid/pull/11233#issuecomment-840940782
thanks for review @jihoonson
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11233:
URL: https://github.com/apache/druid/pull/11233#discussion_r632210107
##########
File path: docs/querying/sql.md
##########
@@ -1022,6 +1022,7 @@ Connection context can be specified as JDBC connection properties or as a "conte
|---------|-----------|-------------|
|`sqlQueryId`|Unique identifier given to this SQL query. For HTTP client, it will be returned in `X-Druid-SQL-Query-Id` header.|auto-generated|
|`sqlTimeZone`|Sets the time zone for this connection, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|druid.sql.planner.sqlTimeZone on the Broker (default: UTC)|
+|`sqlStringifyArrays`|When set to true, result columns which return array values will be serialized into a JSON string in the response instead of as an array (default: true, except for JDBC connections, where it is always false)|
Review comment:
Since there are no other changes I'll try to do this in a follow-up to not churn through CI again
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11233:
URL: https://github.com/apache/druid/pull/11233#discussion_r632811344
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOffsetOperatorConversion.java
##########
@@ -39,16 +38,15 @@
.operandTypeChecker(
OperandTypes.sequence(
"(array,expr)",
- OperandTypes.or(
- OperandTypes.family(SqlTypeFamily.STRING),
- OperandTypes.family(SqlTypeFamily.ARRAY),
- OperandTypes.family(SqlTypeFamily.MULTISET)
- ),
+ OperandTypes.or(
+ OperandTypes.family(SqlTypeFamily.ARRAY),
+ OperandTypes.family(SqlTypeFamily.STRING)
+ ),
OperandTypes.family(SqlTypeFamily.NUMERIC)
Review comment:
ah yeah, that probably could stand to be a bit further restricted
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a change in pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
rohangarg commented on a change in pull request #11233:
URL: https://github.com/apache/druid/pull/11233#discussion_r632359155
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOffsetOperatorConversion.java
##########
@@ -39,16 +38,15 @@
.operandTypeChecker(
OperandTypes.sequence(
"(array,expr)",
- OperandTypes.or(
- OperandTypes.family(SqlTypeFamily.STRING),
- OperandTypes.family(SqlTypeFamily.ARRAY),
- OperandTypes.family(SqlTypeFamily.MULTISET)
- ),
+ OperandTypes.or(
+ OperandTypes.family(SqlTypeFamily.ARRAY),
+ OperandTypes.family(SqlTypeFamily.STRING)
+ ),
OperandTypes.family(SqlTypeFamily.NUMERIC)
Review comment:
should be stricter - like having `SqlTypeFamily.INTEGER`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11233: array handling improvements
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11233:
URL: https://github.com/apache/druid/pull/11233#discussion_r632107206
##########
File path: docs/querying/sql.md
##########
@@ -1022,6 +1022,7 @@ Connection context can be specified as JDBC connection properties or as a "conte
|---------|-----------|-------------|
|`sqlQueryId`|Unique identifier given to this SQL query. For HTTP client, it will be returned in `X-Druid-SQL-Query-Id` header.|auto-generated|
|`sqlTimeZone`|Sets the time zone for this connection, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|druid.sql.planner.sqlTimeZone on the Broker (default: UTC)|
+|`sqlStringifyArrays`|When set to true, result columns which return array values will be serialized into a JSON string in the response instead of as an array (default: true, except for JDBC connections, where it is always false)|
Review comment:
Maybe it will be clearer if it explicitly says this is not overridable in JDBC.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org