[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic parameters URL: https://github.com/apache/druid/pull/6974#discussion_r380450219 ## File path: extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java ## @@ -302,6 +304,7 @@ public void testBloomFilterBigNoParam() throws Exception ); } + @Ignore Review comment: Same comment about adding explanation 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic parameters URL: https://github.com/apache/druid/pull/6974#discussion_r379704878 ## File path: extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java ## @@ -273,10 +275,67 @@ public void testBloomFilters() throws Exception ); } + @Test Review comment: Per https://github.com/apache/druid/pull/6974#issuecomment-459578848, it'd be good to ignore this slow test 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic parameters URL: https://github.com/apache/druid/pull/6974#discussion_r379706863 ## File path: sql/src/main/java/org/apache/druid/sql/http/SqlQuery.java ## @@ -85,13 +110,14 @@ public boolean equals(final Object o) return header == sqlQuery.header && Objects.equals(query, sqlQuery.query) && resultFormat == sqlQuery.resultFormat && - Objects.equals(context, sqlQuery.context); + Objects.equals(context, sqlQuery.context) && Review comment: Suggest adding an `EqualsVerifier` unit test for 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic
parameters
URL: https://github.com/apache/druid/pull/6974#discussion_r379707606
##
File path:
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java
##
@@ -0,0 +1,616 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.avatica.SqlType;
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.aggregation.CountAggregatorFactory;
+import org.apache.druid.query.aggregation.DoubleSumAggregatorFactory;
+import org.apache.druid.query.aggregation.FilteredAggregatorFactory;
+import org.apache.druid.query.dimension.DefaultDimensionSpec;
+import org.apache.druid.query.groupby.GroupByQuery;
+import org.apache.druid.query.ordering.StringComparators;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.sql.calcite.filtration.Filtration;
+import org.apache.druid.sql.calcite.util.CalciteTests;
+import org.apache.druid.sql.http.SqlParameter;
+import org.junit.Test;
+
+/**
+ * This class has copied a subset of the tests in {@link CalciteQueryTest} and
replaced various parts of queries with
+ * dynamic parameters. It is NOT important that this file remains in sync with
{@link CalciteQueryTest}, the tests
+ * were merely chosen to produce a selection of parameter types and positions
within query expressions and have been
+ * renamed to reflect this
+ */
+public class CalciteParameterQueryTest extends BaseCalciteQueryTest
+{
+ private final boolean useDefault = NullHandling.replaceWithDefault();
Review comment:
Would it be beneficial to test binding a null?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic
parameters
URL: https://github.com/apache/druid/pull/6974#discussion_r379706535
##
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java
##
@@ -397,4 +402,44 @@ public static int collapseFetch(int innerFetch, int
outerFetch, int outerOffset)
}
return fetch;
}
+
+ public static Class sqlTypeNameJdbcToJavaClass(SqlTypeName typeName)
+ {
+JDBCType jdbcType = JDBCType.valueOf(typeName.getJdbcOrdinal());
+switch (jdbcType) {
+ case CHAR:
+ case VARCHAR:
+ case LONGVARCHAR:
+return String.class;
+ case NUMERIC:
+ case DECIMAL:
+return BigDecimal.class;
+ case BIT:
+return Boolean.class;
+ case TINYINT:
+return Byte.class;
+ case SMALLINT:
+return Short.class;
+ case INTEGER:
+return Integer.class;
+ case BIGINT:
+return Long.class;
+ case REAL:
+return Float.class;
+ case FLOAT:
Review comment:
I think a comment for why this is not `Float.class` would be 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic
parameters
URL: https://github.com/apache/druid/pull/6974#discussion_r379704623
##
File path: docs/querying/sql.md
##
@@ -607,7 +641,7 @@ Properties connectionProperties = new Properties();
try (Connection connection = DriverManager.getConnection(url,
connectionProperties)) {
try (
final Statement statement = connection.createStatement();
- final ResultSet resultSet = statement.executeQuery(query)
+ final ResultSet resultSet = statement.executeQuery(query);
Review comment:
I think the original example is better as the semicolon is not 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic
parameters
URL: https://github.com/apache/druid/pull/6974#discussion_r379705718
##
File path: sql/src/main/java/org/apache/druid/sql/avatica/DruidStatement.java
##
@@ -152,27 +162,36 @@ public DruidStatement prepare(
try {
ensure(State.NEW);
sqlLifecycle.initialize(query, queryContext);
-sqlLifecycle.planAndAuthorize(authenticationResult);
+
+this.authenticationResult = authenticationResult;
+PrepareResult prepareResult =
sqlLifecycle.prepare(authenticationResult);
this.maxRowCount = maxRowCount;
this.query = query;
+ArrayList params = new ArrayList<>();
Review comment:
Suggestion: Declare as `List` instead of `ArrayList` to separate behavior
from implementation
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #6974: sql support for dynamic parameters
ccaominh commented on a change in pull request #6974: sql support for dynamic
parameters
URL: https://github.com/apache/druid/pull/6974#discussion_r379706847
##
File path: sql/src/main/java/org/apache/druid/sql/http/SqlParameter.java
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.calcite.avatica.ColumnMetaData;
+import org.apache.calcite.avatica.SqlType;
+import org.apache.calcite.avatica.remote.TypedValue;
+import org.apache.calcite.runtime.SqlFunctions;
+import org.apache.calcite.util.TimestampString;
+import org.apache.druid.java.util.common.DateTimes;
+
+import java.sql.Date;
+import java.util.Objects;
+
+public class SqlParameter
+{
+ private final SqlType type;
+ private final Object value;
+
+ @JsonCreator
+ public SqlParameter(
+ @JsonProperty("type") SqlType type,
+ @JsonProperty("value") Object value
+ )
+ {
+this.type = Preconditions.checkNotNull(type);
+this.value = Preconditions.checkNotNull(value);
+ }
+
+ @JsonProperty
+ public Object getValue()
+ {
+return value;
+ }
+
+ @JsonProperty
+ public SqlType getType()
+ {
+return type;
+ }
+
+ @JsonIgnore
+ public TypedValue getTypedValue()
+ {
+
+Object adjustedValue = value;
+
+// perhaps there is a better way to do this?
+if (type == SqlType.TIMESTAMP) {
+ // TypedValue.create for TIMESTAMP expects a long...
+ // but be lenient try to accept iso format and sql 'timestamp' format\
+ if (value instanceof String) {
+try {
+ adjustedValue = DateTimes.of((String) value).getMillis();
+}
+catch (IllegalArgumentException ignore) {
+}
+try {
+ adjustedValue = new TimestampString((String)
value).getMillisSinceEpoch();
+}
+catch (IllegalArgumentException ignore) {
+}
+ }
+} else if (type == SqlType.DATE) {
+ // TypedValue.create for DATE expects calcites internal int
representation of sql dates
+ // but be lenient try to accept sql date '-MM-dd' format and convert
to internal calcite int representation
+ if (value instanceof String) {
+try {
+ adjustedValue = SqlFunctions.toInt(Date.valueOf((String) value));
+}
+catch (IllegalArgumentException ignore) {
+}
+ }
+}
+return
TypedValue.create(ColumnMetaData.Rep.nonPrimitiveRepOf(type).name(),
adjustedValue);
+ }
+
+ @Override
+ public String toString()
+ {
+return "SqlParameter{" +
+ ", value={" + type.name() + ',' + value + '}' +
+ '}';
+ }
+
+ @Override
+ public boolean equals(Object o)
Review comment:
Suggest adding an `EqualsVerifier` unit test for 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
