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 2022/01/12 00:49:11 UTC
[GitHub] [druid] clintropolis opened a new pull request #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
clintropolis opened a new pull request #12145:
URL: https://github.com/apache/druid/pull/12145
### Description
Follow-up to #11949, this PR splits explicit time column functions into standalone `EARLIEST_BY` and `LATEST_BY` SQL functions to avoid the method signatures being ambiguous and dependent on the column types of the inputs.
Prior to this PR, something like `latest(x, 10)` could either translate to "latest" value of x with max bytes of 10 if a string, but if x was a number it would treat the `10` as a timestamp, so instead of a validation exception like would happen prior to #11949, it would allow it but then explode with strange cast exceptions due to unintended behavior on the users part.
Splitting this out into separate functions makes it much less likely for the user to issue the incorrect query.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] 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.
--
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@druid.apache.org
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] gianm commented on a change in pull request #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782664258
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java
##########
@@ -0,0 +1,180 @@
+/*
+ * 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.aggregation.builtin;
+
+import org.apache.calcite.rel.core.AggregateCall;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.util.Optionality;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.aggregation.Aggregation;
+import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.Expressions;
+import org.apache.druid.sql.calcite.planner.Calcites;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry;
+
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class EarliestLatestBySqlAggregator implements SqlAggregator
+{
+ public static final SqlAggregator EARLIEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.EARLIEST);
+ public static final SqlAggregator LATEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.LATEST);
+
+ private final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType;
+ private final SqlAggFunction function;
+
+ private EarliestLatestBySqlAggregator(final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ this.aggregatorType = aggregatorType;
+ this.function = new EarliestByLatestBySqlAggFunction(aggregatorType);
+ }
+
+ @Override
+ public SqlAggFunction calciteFunction()
+ {
+ return function;
+ }
+
+ @Nullable
+ @Override
+ public Aggregation toDruidAggregation(
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final VirtualColumnRegistry virtualColumnRegistry,
+ final RexBuilder rexBuilder,
+ final String name,
+ final AggregateCall aggregateCall,
+ final Project project,
+ final List<Aggregation> existingAggregations,
+ final boolean finalizeAggregations
+ )
+ {
+ final List<RexNode> rexNodes = aggregateCall
+ .getArgList()
+ .stream()
+ .map(i -> Expressions.fromFieldAccess(rowSignature, project, i))
+ .collect(Collectors.toList());
+
+ final List<DruidExpression> args = Expressions.toDruidExpressions(plannerContext, rowSignature, rexNodes);
+
+ if (args == null) {
+ return null;
+ }
+
+ final String aggregatorName = finalizeAggregations ? Calcites.makePrefixedName(name, "a") : name;
+ final ColumnType outputType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
+ if (outputType == null) {
+ throw new ISE(
+ "Cannot translate output sqlTypeName[%s] to Druid type for aggregator[%s]",
+ aggregateCall.getType().getSqlTypeName(),
+ aggregateCall.getName()
+ );
+ }
+
+ final String fieldName = EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(0), rexNodes.get(0));
+
+ final AggregatorFactory theAggFactory;
+ switch (args.size()) {
+ case 2:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(1), rexNodes.get(1)),
+ outputType,
+ -1
+ );
+ break;
+ case 3:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(2), rexNodes.get(2)),
+ outputType,
+ RexLiteral.intValue(rexNodes.get(1))
+ );
+ break;
+ default:
+ throw new IAE(
+ "aggregation[%s], Invalid number of arguments[%,d] to [%s] operator",
+ aggregatorName,
+ args.size(),
+ aggregatorType.name()
+ );
+ }
+
+ return Aggregation.create(
+ Collections.singletonList(theAggFactory),
+ finalizeAggregations ? new FinalizingFieldAccessPostAggregator(name, aggregatorName) : null
+ );
+ }
+
+ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
+ {
+ private static final SqlReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE =
+ new EarliestLatestAnySqlAggregator.EarliestLatestReturnTypeInference(0);
+
+ EarliestByLatestBySqlAggFunction(EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ super(
+ StringUtils.format("%s_BY", aggregatorType.name()),
+ null,
+ SqlKind.OTHER_FUNCTION,
+ EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE,
+ InferTypes.RETURN_TYPE,
+ OperandTypes.or(
+ OperandTypes.sequence(
+ "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ OperandTypes.ANY,
+ OperandTypes.NUMERIC
Review comment:
Hmm, thinking about it more, IMO we should only allow TIMESTAMP. That's consistent with what our other time functions do.
--
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@druid.apache.org
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] gianm commented on a change in pull request #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782660443
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java
##########
@@ -0,0 +1,180 @@
+/*
+ * 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.aggregation.builtin;
+
+import org.apache.calcite.rel.core.AggregateCall;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.util.Optionality;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.aggregation.Aggregation;
+import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.Expressions;
+import org.apache.druid.sql.calcite.planner.Calcites;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry;
+
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class EarliestLatestBySqlAggregator implements SqlAggregator
+{
+ public static final SqlAggregator EARLIEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.EARLIEST);
+ public static final SqlAggregator LATEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.LATEST);
+
+ private final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType;
+ private final SqlAggFunction function;
+
+ private EarliestLatestBySqlAggregator(final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ this.aggregatorType = aggregatorType;
+ this.function = new EarliestByLatestBySqlAggFunction(aggregatorType);
+ }
+
+ @Override
+ public SqlAggFunction calciteFunction()
+ {
+ return function;
+ }
+
+ @Nullable
+ @Override
+ public Aggregation toDruidAggregation(
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final VirtualColumnRegistry virtualColumnRegistry,
+ final RexBuilder rexBuilder,
+ final String name,
+ final AggregateCall aggregateCall,
+ final Project project,
+ final List<Aggregation> existingAggregations,
+ final boolean finalizeAggregations
+ )
+ {
+ final List<RexNode> rexNodes = aggregateCall
+ .getArgList()
+ .stream()
+ .map(i -> Expressions.fromFieldAccess(rowSignature, project, i))
+ .collect(Collectors.toList());
+
+ final List<DruidExpression> args = Expressions.toDruidExpressions(plannerContext, rowSignature, rexNodes);
+
+ if (args == null) {
+ return null;
+ }
+
+ final String aggregatorName = finalizeAggregations ? Calcites.makePrefixedName(name, "a") : name;
+ final ColumnType outputType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
+ if (outputType == null) {
+ throw new ISE(
+ "Cannot translate output sqlTypeName[%s] to Druid type for aggregator[%s]",
+ aggregateCall.getType().getSqlTypeName(),
+ aggregateCall.getName()
+ );
+ }
+
+ final String fieldName = EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(0), rexNodes.get(0));
+
+ final AggregatorFactory theAggFactory;
+ switch (args.size()) {
+ case 2:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(1), rexNodes.get(1)),
+ outputType,
+ -1
+ );
+ break;
+ case 3:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(2), rexNodes.get(2)),
+ outputType,
+ RexLiteral.intValue(rexNodes.get(1))
+ );
+ break;
+ default:
+ throw new IAE(
+ "aggregation[%s], Invalid number of arguments[%,d] to [%s] operator",
+ aggregatorName,
+ args.size(),
+ aggregatorType.name()
+ );
+ }
+
+ return Aggregation.create(
+ Collections.singletonList(theAggFactory),
+ finalizeAggregations ? new FinalizingFieldAccessPostAggregator(name, aggregatorName) : null
+ );
+ }
+
+ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
+ {
+ private static final SqlReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE =
+ new EarliestLatestAnySqlAggregator.EarliestLatestReturnTypeInference(0);
+
+ EarliestByLatestBySqlAggFunction(EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ super(
+ StringUtils.format("%s_BY", aggregatorType.name()),
+ null,
+ SqlKind.OTHER_FUNCTION,
+ EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE,
+ InferTypes.RETURN_TYPE,
+ OperandTypes.or(
+ OperandTypes.sequence(
+ "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ OperandTypes.ANY,
+ OperandTypes.NUMERIC
Review comment:
IMO we should allow either NUMERIC or TIMESTAMP here.
--
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@druid.apache.org
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 #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #12145:
URL: https://github.com/apache/druid/pull/12145
--
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@druid.apache.org
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 #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782661646
##########
File path: docs/querying/sql.md
##########
@@ -365,13 +365,13 @@ In the aggregation functions supported by Druid, only `COUNT`, `ARRAY_AGG`, and
|`STDDEV_SAMP(expr)`|Computes standard deviation sample of `expr`. See [stats extension](../development/extensions-core/stats.md) documentation for additional details.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`STDDEV(expr)`|Computes standard deviation sample of `expr`. See [stats extension](../development/extensions-core/stats.md) documentation for additional details.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`EARLIEST(expr)`|Returns the earliest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "earliest" is the value first encountered with the minimum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the first value encountered.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
-|`EARLIEST(expr, timeColumn)`|Returns the earliest value of `expr`, which must be numeric. Earliest value is defined as the value first encountered with the minimum overall value of time column of all values being aggregated.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
-|`EARLIEST(expr, maxBytesPerString, timeColumn)`|Like `EARLIEST(expr, timeColumn)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
+|`EARLIEST_BY(expr, timeColumn)`|Returns the earliest value of `expr`, which must be numeric. Earliest value is defined as the value first encountered with the minimum overall value of time column of all values being aggregated.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
+|`EARLIEST_BY(expr, maxBytesPerString, timeColumn)`|Like `EARLIEST_BY(expr, timeColumn)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
Review comment:
agreed, changed
--
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@druid.apache.org
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 #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782746783
##########
File path: .idea/inspectionProfiles/Druid.xml
##########
@@ -70,7 +70,6 @@
</option>
<option name="IGNORE_FIELDS_USED_IN_MULTIPLE_METHODS" value="true" />
</inspection_tool>
- <inspection_tool class="FieldMayBeFinal" enabled="true" level="WARNING" enabled_by_default="true" />
Review comment:
no heh, will fix
--
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@druid.apache.org
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 #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782719975
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java
##########
@@ -0,0 +1,180 @@
+/*
+ * 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.aggregation.builtin;
+
+import org.apache.calcite.rel.core.AggregateCall;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.util.Optionality;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.aggregation.Aggregation;
+import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.Expressions;
+import org.apache.druid.sql.calcite.planner.Calcites;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry;
+
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class EarliestLatestBySqlAggregator implements SqlAggregator
+{
+ public static final SqlAggregator EARLIEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.EARLIEST);
+ public static final SqlAggregator LATEST_BY = new EarliestLatestBySqlAggregator(EarliestLatestAnySqlAggregator.AggregatorType.LATEST);
+
+ private final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType;
+ private final SqlAggFunction function;
+
+ private EarliestLatestBySqlAggregator(final EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ this.aggregatorType = aggregatorType;
+ this.function = new EarliestByLatestBySqlAggFunction(aggregatorType);
+ }
+
+ @Override
+ public SqlAggFunction calciteFunction()
+ {
+ return function;
+ }
+
+ @Nullable
+ @Override
+ public Aggregation toDruidAggregation(
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final VirtualColumnRegistry virtualColumnRegistry,
+ final RexBuilder rexBuilder,
+ final String name,
+ final AggregateCall aggregateCall,
+ final Project project,
+ final List<Aggregation> existingAggregations,
+ final boolean finalizeAggregations
+ )
+ {
+ final List<RexNode> rexNodes = aggregateCall
+ .getArgList()
+ .stream()
+ .map(i -> Expressions.fromFieldAccess(rowSignature, project, i))
+ .collect(Collectors.toList());
+
+ final List<DruidExpression> args = Expressions.toDruidExpressions(plannerContext, rowSignature, rexNodes);
+
+ if (args == null) {
+ return null;
+ }
+
+ final String aggregatorName = finalizeAggregations ? Calcites.makePrefixedName(name, "a") : name;
+ final ColumnType outputType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
+ if (outputType == null) {
+ throw new ISE(
+ "Cannot translate output sqlTypeName[%s] to Druid type for aggregator[%s]",
+ aggregateCall.getType().getSqlTypeName(),
+ aggregateCall.getName()
+ );
+ }
+
+ final String fieldName = EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(0), rexNodes.get(0));
+
+ final AggregatorFactory theAggFactory;
+ switch (args.size()) {
+ case 2:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(1), rexNodes.get(1)),
+ outputType,
+ -1
+ );
+ break;
+ case 3:
+ theAggFactory = aggregatorType.createAggregatorFactory(
+ aggregatorName,
+ fieldName,
+ EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(2), rexNodes.get(2)),
+ outputType,
+ RexLiteral.intValue(rexNodes.get(1))
+ );
+ break;
+ default:
+ throw new IAE(
+ "aggregation[%s], Invalid number of arguments[%,d] to [%s] operator",
+ aggregatorName,
+ args.size(),
+ aggregatorType.name()
+ );
+ }
+
+ return Aggregation.create(
+ Collections.singletonList(theAggFactory),
+ finalizeAggregations ? new FinalizingFieldAccessPostAggregator(name, aggregatorName) : null
+ );
+ }
+
+ private static class EarliestByLatestBySqlAggFunction extends SqlAggFunction
+ {
+ private static final SqlReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE =
+ new EarliestLatestAnySqlAggregator.EarliestLatestReturnTypeInference(0);
+
+ EarliestByLatestBySqlAggFunction(EarliestLatestAnySqlAggregator.AggregatorType aggregatorType)
+ {
+ super(
+ StringUtils.format("%s_BY", aggregatorType.name()),
+ null,
+ SqlKind.OTHER_FUNCTION,
+ EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE,
+ InferTypes.RETURN_TYPE,
+ OperandTypes.or(
+ OperandTypes.sequence(
+ "'" + aggregatorType.name() + "(expr, timeColumn)'\n",
+ OperandTypes.ANY,
+ OperandTypes.NUMERIC
Review comment:
changed, and this found a bug when i had to switch to column that could actually be converted to TIMESTAMP (a long), we weren't correctly checking if the time column selector had null values, so I've fixed up the native aggregators to handle this case.
--
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@druid.apache.org
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] gianm commented on a change in pull request #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782743657
##########
File path: .idea/inspectionProfiles/Druid.xml
##########
@@ -70,7 +70,6 @@
</option>
<option name="IGNORE_FIELDS_USED_IN_MULTIPLE_METHODS" value="true" />
</inspection_tool>
- <inspection_tool class="FieldMayBeFinal" enabled="true" level="WARNING" enabled_by_default="true" />
Review comment:
Was this change (& the others in this file) intentional?
--
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@druid.apache.org
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] imply-cheddar commented on a change in pull request #12145: add EARLIEST_BY/LATEST_BY to make EARLIEST/LATEST function signatures less ambiguous
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on a change in pull request #12145:
URL: https://github.com/apache/druid/pull/12145#discussion_r782659713
##########
File path: docs/querying/sql.md
##########
@@ -365,13 +365,13 @@ In the aggregation functions supported by Druid, only `COUNT`, `ARRAY_AGG`, and
|`STDDEV_SAMP(expr)`|Computes standard deviation sample of `expr`. See [stats extension](../development/extensions-core/stats.md) documentation for additional details.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`STDDEV(expr)`|Computes standard deviation sample of `expr`. See [stats extension](../development/extensions-core/stats.md) documentation for additional details.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`EARLIEST(expr)`|Returns the earliest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "earliest" is the value first encountered with the minimum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the first value encountered.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
-|`EARLIEST(expr, timeColumn)`|Returns the earliest value of `expr`, which must be numeric. Earliest value is defined as the value first encountered with the minimum overall value of time column of all values being aggregated.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
|`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
-|`EARLIEST(expr, maxBytesPerString, timeColumn)`|Like `EARLIEST(expr, timeColumn)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
+|`EARLIEST_BY(expr, timeColumn)`|Returns the earliest value of `expr`, which must be numeric. Earliest value is defined as the value first encountered with the minimum overall value of time column of all values being aggregated.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `0`|
+|`EARLIEST_BY(expr, maxBytesPerString, timeColumn)`|Like `EARLIEST_BY(expr, timeColumn)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.|`null` if `druid.generic.useDefaultValueForNull=false`, otherwise `''`|
Review comment:
With `EARLIEST_BY` I'd suggest we swap the location of `timeColumn` and `maxBytesPerString`. Given that `timeColumn` is non-optional on the `_BY` items, we want the optional parameter to be at the end of the signature.
--
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@druid.apache.org
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