github-advanced-security[bot] commented on code in PR #16402: URL: https://github.com/apache/druid/pull/16402#discussion_r1591253596
########## sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateProjectMergeRule.java: ########## @@ -0,0 +1,197 @@ +/* + * 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.rule.logical; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.plan.RelRule; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Aggregate; +import org.apache.calcite.rel.core.Aggregate.Group; +import org.apache.calcite.rel.core.AggregateCall; +import org.apache.calcite.rel.core.Project; +import org.apache.calcite.rel.rules.CoreRules; +import org.apache.calcite.rel.rules.TransformationRule; +import org.apache.calcite.rex.RexInputRef; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.RelBuilderFactory; +import org.apache.calcite.util.ImmutableBitSet; +import org.apache.calcite.util.Util; +import org.apache.calcite.util.mapping.Mappings; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.immutables.value.Value; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; + +/** + * Planner rule that recognizes a {@link Aggregate} + * on top of a {@link Project} and if possible + * aggregate through the project or removes the project. + * + * This is updated version of {@link org.apache.calcite.rel.rules.AggregateProjectMergeRule} + * to be able to handle expressions + */ [email protected] +public class DruidAggregateProjectMergeRule + extends RelRule<DruidAggregateProjectMergeRule.Config> + implements TransformationRule +{ + + /** + * Creates a DruidAggregateProjectMergeRule. + */ + protected DruidAggregateProjectMergeRule(Config config) + { + super(config); + } + + @Deprecated // to be removed before 2.0 + public DruidAggregateProjectMergeRule( + Class<? extends Aggregate> aggregateClass, + Class<? extends Project> projectClass, + RelBuilderFactory relBuilderFactory + ) + { + this(CoreRules.AGGREGATE_PROJECT_MERGE.config + .withRelBuilderFactory(relBuilderFactory) + .as(Config.class) + .withOperandFor(aggregateClass, projectClass)); + } + + @Override + public void onMatch(RelOptRuleCall call) + { + final Aggregate aggregate = call.rel(0); + final Project project = call.rel(1); + RelNode x = apply(call, aggregate, project); + if (x != null) { + call.transformTo(x); + } + } + + public static @Nullable RelNode apply(RelOptRuleCall call, Aggregate aggregate, Project project) + { + final Set<Integer> interestingFields = RelOptUtil.getAllFields(aggregate); + if (interestingFields.isEmpty()) { + return null; + } + final Map<Integer, Integer> map = new HashMap<>(); + final Map<RexNode, Integer> assignedNodeForExpr = new HashMap<>(); + List<RexNode> newRexNodes = new ArrayList<>(); + for (int source : interestingFields) { + final RexNode rex = project.getProjects().get(source); + if (!assignedNodeForExpr.containsKey(rex)) { + RexNode newNode = new RexInputRef(source, rex.getType()); + assignedNodeForExpr.put(rex, newRexNodes.size()); + newRexNodes.add(newNode); + } + map.put(source, assignedNodeForExpr.get(rex)); + } + + final ImmutableBitSet newGroupSet = aggregate.getGroupSet().permute(map); + ImmutableList<ImmutableBitSet> newGroupingSets = null; + if (aggregate.getGroupType() != Group.SIMPLE) { + newGroupingSets = + ImmutableBitSet.ORDERING.immutableSortedCopy( + Sets.newTreeSet(ImmutableBitSet.permute(aggregate.getGroupSets(), map))); + } + + final ImmutableList.Builder<AggregateCall> aggCalls = ImmutableList.builder(); + final int sourceCount = aggregate.getInput().getRowType().getFieldCount(); + final int targetCount = newRexNodes.size(); + final Mappings.TargetMapping targetMapping = Mappings.target(map, sourceCount, targetCount); + for (AggregateCall aggregateCall : aggregate.getAggCallList()) { + aggCalls.add(aggregateCall.transform(targetMapping)); + } + + final RelBuilder relBuilder = call.builder(); + relBuilder.push(project); + relBuilder.project(newRexNodes); + + final Aggregate newAggregate = + aggregate.copy(aggregate.getTraitSet(), relBuilder.build(), + newGroupSet, newGroupingSets, aggCalls.build() + ); + relBuilder.push(newAggregate); + + final List<Integer> newKeys = + Util.transform( + aggregate.getGroupSet().asList(), + key -> Objects.requireNonNull( + map.get(key), + () -> "no value found for key " + key + " in " + map + ) + ); + + // Add a project if the group set is not in the same order or + // contains duplicates. + if (!newKeys.equals(newGroupSet.asList())) { + final List<Integer> posList = new ArrayList<>(); + for (int newKey : newKeys) { + posList.add(newGroupSet.indexOf(newKey)); + } + for (int i = newAggregate.getGroupCount(); + i < newAggregate.getRowType().getFieldCount(); i++) { + posList.add(i); + } + relBuilder.project(relBuilder.fields(posList)); + } + + return relBuilder.build(); + } + + /** + * Rule configuration. + */ + @Value.Immutable + public interface Config extends RelRule.Config Review Comment: ## Class has same name as super class Config has the same name as its supertype [org.apache.calcite.plan.RelRule$Config](1). [Show more details](https://github.com/apache/druid/security/code-scanning/7342) ########## sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java: ########## @@ -0,0 +1,391 @@ +/* + * 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.rule.logical; + +import com.google.common.base.MoreObjects; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import org.apache.calcite.plan.RelRule; +import org.apache.calcite.rel.core.Aggregate; +import org.apache.calcite.rel.core.Project; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.tools.RelBuilderFactory; +import org.immutables.value.Generated; + +import javax.annotation.CheckReturnValue; +import javax.annotation.Nullable; +import javax.annotation.ParametersAreNonnullByDefault; +import javax.annotation.concurrent.Immutable; +import javax.annotation.concurrent.NotThreadSafe; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + +/** + * Custom version of {@link org.apache.calcite.rel.rules.ImmutableAggregateProjectMergeRule} + */ +@ParametersAreNonnullByDefault +@Generated( + from = "DruidAggregateProjectMergeRule", + generator = "Immutables" +) +final class DruidImmutableAggregateProjectMergeRule +{ + private DruidImmutableAggregateProjectMergeRule() + { + } + + @CheckReturnValue + @Generated( + from = "DruidAggregateProjectMergeRule.Config", + generator = "Immutables" + ) + @Immutable + static final class Config implements DruidAggregateProjectMergeRule.Config + { + private final RelBuilderFactory relBuilderFactory; + @Nullable + private final @org.checkerframework.checker.nullness.qual.Nullable String description; + private final RelRule.OperandTransform operandSupplier; + private static final byte STAGE_INITIALIZING = -1; + private static final byte STAGE_UNINITIALIZED = 0; + private static final byte STAGE_INITIALIZED = 1; + private transient volatile InitShim initShim; + private static final Config INSTANCE = validate(new Config()); + + private Config() + { + this.initShim = new InitShim(); + this.description = null; + this.relBuilderFactory = this.initShim.relBuilderFactory(); + this.operandSupplier = this.initShim.operandSupplier(); + this.initShim = null; + } + + private Config(Builder builder) + { + this.initShim = new InitShim(); + this.description = builder.description; + if (builder.relBuilderFactory != null) { + this.initShim.withRelBuilderFactory(builder.relBuilderFactory); + } + + if (builder.operandSupplier != null) { + this.initShim.withOperandSupplier(builder.operandSupplier); + } + + this.relBuilderFactory = this.initShim.relBuilderFactory(); + this.operandSupplier = this.initShim.operandSupplier(); + this.initShim = null; + } + + private Config( + RelBuilderFactory relBuilderFactory, + @Nullable @org.checkerframework.checker.nullness.qual.Nullable String description, + RelRule.OperandTransform operandSupplier + ) + { + this.initShim = new InitShim(); + this.relBuilderFactory = relBuilderFactory; + this.description = description; + this.operandSupplier = operandSupplier; + this.initShim = null; + } + + private RelBuilderFactory relBuilderFactoryInitialize() + { + return RelFactories.LOGICAL_BUILDER; + } + + private RelRule.OperandTransform operandSupplierInitialize() + { + return s -> { + throw new IllegalArgumentException("Rules must have at least one " + + "operand. Call Config.withOperandSupplier to specify them."); + }; + } + + @Override + public RelBuilderFactory relBuilderFactory() + { + InitShim shim = this.initShim; + return shim != null ? shim.relBuilderFactory() : this.relBuilderFactory; + } + + @Override + @Nullable + public @org.checkerframework.checker.nullness.qual.Nullable String description() + { + return this.description; + } + + @Override + public RelRule.OperandTransform operandSupplier() + { + InitShim shim = this.initShim; + return shim != null ? shim.operandSupplier() : this.operandSupplier; + } + + @Override + public Config withRelBuilderFactory(RelBuilderFactory value) + { + if (this.relBuilderFactory == value) { + return this; + } else { + RelBuilderFactory newValue = (RelBuilderFactory) Objects.requireNonNull(value, "relBuilderFactory"); + return validate(new Config(newValue, this.description, this.operandSupplier)); + } + } + + @Override + public Config withDescription(@Nullable @org.checkerframework.checker.nullness.qual.Nullable String value) + { + return Objects.equals(this.description, value) + ? this + : validate(new Config(this.relBuilderFactory, value, this.operandSupplier)); + } + + @Override + public Config withOperandSupplier(RelRule.OperandTransform value) + { + if (this.operandSupplier == value) { + return this; + } else { + RelRule.OperandTransform newValue = (RelRule.OperandTransform) Objects.requireNonNull(value, "operandSupplier"); + return validate(new Config(this.relBuilderFactory, this.description, newValue)); + } + } + + @Override + public boolean equals(@Nullable Object another) + { + if (this == another) { + return true; + } else { + return another instanceof Config && this.equalTo((Config) another); + } + } + + private boolean equalTo(Config another) + { + return this.relBuilderFactory.equals(another.relBuilderFactory) && Objects.equals( + this.description, + another.description + ) && this.operandSupplier.equals(another.operandSupplier); + } + + @Override + public int hashCode() + { + int h = 5381; + h += (h << 5) + this.relBuilderFactory.hashCode(); + h += (h << 5) + Objects.hashCode(this.description); + h += (h << 5) + this.operandSupplier.hashCode(); + return h; + } + + @Override + public String toString() + { + return MoreObjects.toStringHelper("Config") + .omitNullValues() + .add("relBuilderFactory", this.relBuilderFactory) + .add("description", this.description) + .add("operandSupplier", this.operandSupplier) + .toString(); + } + + public static Config of() + { + return INSTANCE; + } + + private static Config validate(Config instance) + { + return INSTANCE != null && INSTANCE.equalTo(instance) ? INSTANCE : instance; + } + + public static Config copyOf(DruidAggregateProjectMergeRule.Config instance) + { + return instance instanceof Config ? (Config) instance : builder().from(instance).build(); + } + + public static Builder builder() + { + return new Builder(); + } + + @Generated( + from = "DruidAggregateProjectMergeRule.Config", + generator = "Immutables" + ) + @NotThreadSafe + public static final class Builder + { + @Nullable + private RelBuilderFactory relBuilderFactory; + @Nullable + private @org.checkerframework.checker.nullness.qual.Nullable String description; + @Nullable + private RelRule.OperandTransform operandSupplier; + + private Builder() + { + } + + @CanIgnoreReturnValue + public Builder from(RelRule.Config instance) + { + Objects.requireNonNull(instance, "instance"); + this.from((Object) instance); + return this; + } + + @CanIgnoreReturnValue + public Builder from(DruidAggregateProjectMergeRule.Config instance) + { + Objects.requireNonNull(instance, "instance"); + this.from((Object) instance); + return this; + } + + private void from(Object object) Review Comment: ## Confusing overloading of methods Method Builder.from(..) could be confused with overloaded method [from](1), since dispatch depends on static types. Method Builder.from(..) could be confused with overloaded method [from](2), since dispatch depends on static types. [Show more details](https://github.com/apache/druid/security/code-scanning/7341) ########## sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java: ########## @@ -0,0 +1,391 @@ +/* + * 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.rule.logical; + +import com.google.common.base.MoreObjects; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import org.apache.calcite.plan.RelRule; +import org.apache.calcite.rel.core.Aggregate; +import org.apache.calcite.rel.core.Project; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.tools.RelBuilderFactory; +import org.immutables.value.Generated; + +import javax.annotation.CheckReturnValue; +import javax.annotation.Nullable; +import javax.annotation.ParametersAreNonnullByDefault; +import javax.annotation.concurrent.Immutable; +import javax.annotation.concurrent.NotThreadSafe; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + +/** + * Custom version of {@link org.apache.calcite.rel.rules.ImmutableAggregateProjectMergeRule} + */ +@ParametersAreNonnullByDefault +@Generated( + from = "DruidAggregateProjectMergeRule", + generator = "Immutables" +) +final class DruidImmutableAggregateProjectMergeRule +{ + private DruidImmutableAggregateProjectMergeRule() + { + } + + @CheckReturnValue + @Generated( + from = "DruidAggregateProjectMergeRule.Config", + generator = "Immutables" + ) + @Immutable + static final class Config implements DruidAggregateProjectMergeRule.Config Review Comment: ## Class has same name as super class Config has the same name as its supertype [org.apache.druid.sql.calcite.rule.logical.DruidAggregateProjectMergeRule$Config](1). [Show more details](https://github.com/apache/druid/security/code-scanning/7343) ########## sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java: ########## @@ -0,0 +1,391 @@ +/* + * 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.rule.logical; + +import com.google.common.base.MoreObjects; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import org.apache.calcite.plan.RelRule; +import org.apache.calcite.rel.core.Aggregate; +import org.apache.calcite.rel.core.Project; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.tools.RelBuilderFactory; +import org.immutables.value.Generated; + +import javax.annotation.CheckReturnValue; +import javax.annotation.Nullable; +import javax.annotation.ParametersAreNonnullByDefault; +import javax.annotation.concurrent.Immutable; +import javax.annotation.concurrent.NotThreadSafe; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + +/** + * Custom version of {@link org.apache.calcite.rel.rules.ImmutableAggregateProjectMergeRule} + */ +@ParametersAreNonnullByDefault +@Generated( + from = "DruidAggregateProjectMergeRule", + generator = "Immutables" +) +final class DruidImmutableAggregateProjectMergeRule +{ + private DruidImmutableAggregateProjectMergeRule() + { + } + + @CheckReturnValue + @Generated( + from = "DruidAggregateProjectMergeRule.Config", + generator = "Immutables" + ) + @Immutable + static final class Config implements DruidAggregateProjectMergeRule.Config + { + private final RelBuilderFactory relBuilderFactory; + @Nullable + private final @org.checkerframework.checker.nullness.qual.Nullable String description; + private final RelRule.OperandTransform operandSupplier; + private static final byte STAGE_INITIALIZING = -1; + private static final byte STAGE_UNINITIALIZED = 0; + private static final byte STAGE_INITIALIZED = 1; + private transient volatile InitShim initShim; + private static final Config INSTANCE = validate(new Config()); + + private Config() + { + this.initShim = new InitShim(); + this.description = null; + this.relBuilderFactory = this.initShim.relBuilderFactory(); + this.operandSupplier = this.initShim.operandSupplier(); + this.initShim = null; + } + + private Config(Builder builder) + { + this.initShim = new InitShim(); + this.description = builder.description; + if (builder.relBuilderFactory != null) { + this.initShim.withRelBuilderFactory(builder.relBuilderFactory); + } + + if (builder.operandSupplier != null) { + this.initShim.withOperandSupplier(builder.operandSupplier); + } + + this.relBuilderFactory = this.initShim.relBuilderFactory(); + this.operandSupplier = this.initShim.operandSupplier(); + this.initShim = null; + } + + private Config( + RelBuilderFactory relBuilderFactory, + @Nullable @org.checkerframework.checker.nullness.qual.Nullable String description, + RelRule.OperandTransform operandSupplier + ) + { + this.initShim = new InitShim(); + this.relBuilderFactory = relBuilderFactory; + this.description = description; + this.operandSupplier = operandSupplier; + this.initShim = null; + } + + private RelBuilderFactory relBuilderFactoryInitialize() + { + return RelFactories.LOGICAL_BUILDER; + } + + private RelRule.OperandTransform operandSupplierInitialize() + { + return s -> { + throw new IllegalArgumentException("Rules must have at least one " + + "operand. Call Config.withOperandSupplier to specify them."); + }; + } + + @Override + public RelBuilderFactory relBuilderFactory() + { + InitShim shim = this.initShim; + return shim != null ? shim.relBuilderFactory() : this.relBuilderFactory; + } + + @Override + @Nullable + public @org.checkerframework.checker.nullness.qual.Nullable String description() + { + return this.description; + } + + @Override + public RelRule.OperandTransform operandSupplier() + { + InitShim shim = this.initShim; + return shim != null ? shim.operandSupplier() : this.operandSupplier; + } + + @Override + public Config withRelBuilderFactory(RelBuilderFactory value) + { + if (this.relBuilderFactory == value) { + return this; + } else { + RelBuilderFactory newValue = (RelBuilderFactory) Objects.requireNonNull(value, "relBuilderFactory"); + return validate(new Config(newValue, this.description, this.operandSupplier)); + } + } + + @Override + public Config withDescription(@Nullable @org.checkerframework.checker.nullness.qual.Nullable String value) + { + return Objects.equals(this.description, value) + ? this + : validate(new Config(this.relBuilderFactory, value, this.operandSupplier)); + } + + @Override + public Config withOperandSupplier(RelRule.OperandTransform value) + { + if (this.operandSupplier == value) { + return this; + } else { + RelRule.OperandTransform newValue = (RelRule.OperandTransform) Objects.requireNonNull(value, "operandSupplier"); + return validate(new Config(this.relBuilderFactory, this.description, newValue)); + } + } + + @Override + public boolean equals(@Nullable Object another) + { + if (this == another) { + return true; + } else { + return another instanceof Config && this.equalTo((Config) another); + } + } + + private boolean equalTo(Config another) + { + return this.relBuilderFactory.equals(another.relBuilderFactory) && Objects.equals( + this.description, + another.description + ) && this.operandSupplier.equals(another.operandSupplier); + } + + @Override + public int hashCode() + { + int h = 5381; + h += (h << 5) + this.relBuilderFactory.hashCode(); + h += (h << 5) + Objects.hashCode(this.description); + h += (h << 5) + this.operandSupplier.hashCode(); + return h; + } + + @Override + public String toString() + { + return MoreObjects.toStringHelper("Config") + .omitNullValues() + .add("relBuilderFactory", this.relBuilderFactory) + .add("description", this.description) + .add("operandSupplier", this.operandSupplier) + .toString(); + } + + public static Config of() + { + return INSTANCE; + } + + private static Config validate(Config instance) + { + return INSTANCE != null && INSTANCE.equalTo(instance) ? INSTANCE : instance; + } + + public static Config copyOf(DruidAggregateProjectMergeRule.Config instance) + { + return instance instanceof Config ? (Config) instance : builder().from(instance).build(); + } + + public static Builder builder() + { + return new Builder(); + } + + @Generated( + from = "DruidAggregateProjectMergeRule.Config", + generator = "Immutables" + ) + @NotThreadSafe + public static final class Builder + { + @Nullable + private RelBuilderFactory relBuilderFactory; + @Nullable + private @org.checkerframework.checker.nullness.qual.Nullable String description; + @Nullable + private RelRule.OperandTransform operandSupplier; + + private Builder() + { + } + + @CanIgnoreReturnValue + public Builder from(RelRule.Config instance) Review Comment: ## Confusing overloading of methods Method Builder.from(..) could be confused with overloaded method [from](1), since dispatch depends on static types. [Show more details](https://github.com/apache/druid/security/code-scanning/7340) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
