zabetak commented on code in PR #5196:
URL: https://github.com/apache/hive/pull/5196#discussion_r1928847917
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java:
##########
@@ -77,6 +77,7 @@
import org.apache.calcite.util.mapping.Mappings;
import org.apache.hadoop.hive.ql.exec.FunctionRegistry;
import org.apache.hadoop.hive.ql.optimizer.calcite.reloperators.HiveProject;
+import
org.apache.hadoop.hive.ql.optimizer.calcite.rules.HivePointLookupOptimizerRule;
Review Comment:
It's strange to have a utility class depend on a rule. If this method is
useful outside the rule then consider moving the dependent parts here or
extract them to top-level package private classes.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java:
##########
@@ -224,7 +224,7 @@ public void onMatch(RelOptRuleCall call) {
}
}
side.newInput = relBuilder.push(joinInput)
- .aggregate(relBuilder.groupKey(belowAggregateKey, null),
+ .aggregate(relBuilder.groupKey(belowAggregateKey,
ImmutableList.of(belowAggregateKey)),
Review Comment:
Is it equivalent if we use `relbuilder.groupKey(belowAggregateKey)`?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/Bug.java:
##########
@@ -29,6 +29,8 @@
*
*/
public final class Bug {
+
+ public static final int CALCITE_VERSION = 33;
Review Comment:
Let's use specific JIRA ids instead of general version numbers.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveProject.java:
##########
@@ -138,6 +143,18 @@ public boolean isSynthetic() {
@Override
public RelWriter explainTerms(RelWriter pw) {
+ // Remove this if block after upgrading Calcite to 1.35+
+ if (CALCITE_VERSION < 35
+ && pw.getDetailLevel() == SqlExplainLevel.ALL_ATTRIBUTES &&
getDigest().contains("SEARCH")) {
+ return ((HiveProject) this.accept(
+ RexUtil.searchShuttle(
+ new RexBuilder(new JavaTypeFactoryImpl(new
HiveTypeSystemImpl())),
+ null,
+ -1
+ )
+ )).explainTerms(pw);
+ }
Review Comment:
I assume this block is to address the problem of serializing SEARCH op to
JSON. If that's the case then it may preferable to do the expansion inside
`HiveRelOptUtil#toJsonString` in a more generic way that its not operator
specific.
Another alternative would be to modify `HiveRelJson` classes and apply the
fix as done in Calcite.
##########
ql/src/test/queries/clientpositive/pcs.q:
##########
@@ -58,7 +58,7 @@ SORT BY A.key, A.value, A.ds;
explain extended select ds from pcs_t1 where struct(case when ds='2000-04-08'
then 10 else 20 end) in (struct(10),struct(11));
select ds from pcs_t1 where struct(case when ds='2000-04-08' then 10 else 20
end) in (struct(10),struct(11));
-explain extended select ds from pcs_t1 where struct(ds, key, rand(100)) in
(struct('2000-04-08',1,0.2), struct('2000-04-09',2,0.3));
+explain extended select ds from pcs_t1 where struct(ds, key) in
(struct('2000-04-08',1), struct('2000-04-09',2));
Review Comment:
Once we are ready to merge this PR we should create a JIRA ticket to
document the regression and potentially handle it in a follow-up.
##########
ql/src/test/queries/clientpositive/pointlookup3.q:
##########
@@ -1,4 +1,6 @@
--! qt:dataset:src
+-- SORT_QUERY_RESULTS
+
Review Comment:
Let's commit this change under a separate ticket since it is independent
with the update.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/functions/HiveSqlAggFunction.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.hadoop.hive.ql.optimizer.calcite.functions;
+
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlOperandTypeInference;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.util.Optionality;
+import org.apache.hadoop.hive.ql.optimizer.calcite.HiveRelBuilder;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class HiveSqlAggFunction extends SqlAggFunction {
Review Comment:
Why do we need this change? Is it specific to the upgrade? Do you remember
@kasakrisz ?
##########
ql/src/test/queries/clientpositive/pcs.q:
##########
@@ -58,7 +58,7 @@ SORT BY A.key, A.value, A.ds;
explain extended select ds from pcs_t1 where struct(case when ds='2000-04-08'
then 10 else 20 end) in (struct(10),struct(11));
select ds from pcs_t1 where struct(case when ds='2000-04-08' then 10 else 20
end) in (struct(10),struct(11));
-explain extended select ds from pcs_t1 where struct(ds, key, rand(100)) in
(struct('2000-04-08',1,0.2), struct('2000-04-09',2,0.3));
+explain extended select ds from pcs_t1 where struct(ds, key) in
(struct('2000-04-08',1), struct('2000-04-09',2));
Review Comment:
If we remove the RAND we are losing in terms of test coverage which was
actually useful to detect this regression. We should strive to increase
coverage not the other way around.
--
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]
