zabetak commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434034182
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptMaterializationValidator.java:
##########
@@ -308,24 +318,19 @@ public boolean isValidForQueryCaching() {
return resultCacheInvalidReason == null;
}
- public String getAutomaticRewritingInvalidReason() {
- return automaticRewritingInvalidReason;
- }
-
- public void setAutomaticRewritingInvalidReason(String
automaticRewritingInvalidReason) {
- if (isValidForAutomaticRewriting()) {
- this.automaticRewritingInvalidReason = automaticRewritingInvalidReason;
- }
+ public AutomaticRewritingValidationResult
getAutomaticRewritingValidationResult() {
+ return automaticRewritingValidationResult;
}
- public void setAutomaticRewritingInvalidReason(RelNode node) {
+ public void unsupportedByCalciteRewrite(String sqlPartType, String sqlPart) {
if (isValidForAutomaticRewriting()) {
Review Comment:
Why is this if check necessary? Are we interested only on the first failure?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptMaterializationValidator.java:
##########
@@ -62,13 +65,16 @@
* - References to non-deterministic functions.
*/
public class HiveRelOptMaterializationValidator extends HiveRelShuttleImpl {
- static final Logger LOG =
LoggerFactory.getLogger(HiveRelOptMaterializationValidator.class);
+ public static final String UNSUPPORTED_BY_CALCITE_FORMAT =
Review Comment:
public -> private
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14668,11 +14669,9 @@ private void validateCreateView()
}
throw new SemanticException(msg);
}
- if (!isValidAutomaticRewritingMaterialization()) {
- String errorMessage = "Only query text based automatic rewriting is
available for materialized view. " +
- getInvalidAutomaticRewritingMaterializationReason();
- console.printError(errorMessage);
- LOG.warn(errorMessage);
+ String validationWarningMessage =
automaticRewritingValidationResult.getErrorMessage();
Review Comment:
The name of the variable is a bit misleading. We are printing this as an
error in the console so not sure why the name includes warning.
##########
ql/src/test/org/apache/hadoop/hive/ql/metadata/TestMaterializedViewsCache.java:
##########
@@ -157,7 +157,7 @@ private Table getTable(String db, String tableName, String
definition) {
private static HiveRelOptMaterialization createMaterialization(Table table)
throws ParseException {
return new HiveRelOptMaterialization(
new DummyRel(table), new DummyRel(table), null,
asList(table.getDbName(), table.getTableName()),
- EnumSet.allOf(HiveRelOptMaterialization.RewriteAlgorithm.class),
+ EnumSet.allOf(RewriteAlgorithm.class),
Review Comment:
Should we use RewriteAlgorithm.ALL instead?
##########
ql/src/test/results/clientpositive/llap/materialized_view_no_cbo_rewrite.q.out:
##########
@@ -36,4 +36,8 @@ PREHOOK: query: alter materialized view cmv_mat_view enable
rewrite
PREHOOK: type: ALTER_MATERIALIZED_VIEW_REWRITE
PREHOOK: Input: default@cmv_mat_view
PREHOOK: Output: default@cmv_mat_view
-FAILED: Execution Error, return code 40000 from
org.apache.hadoop.hive.ql.ddl.DDLTask.
org.apache.hadoop.hive.ql.metadata.HiveException: Cannot enable rewriting for
materialized view. Statement has unsupported clause: sort by.
+Only query text based automatic rewriting is available for materialized view.
Statement has unsupported clause: sort by.
Review Comment:
It seems that with this patch we also enable more rewritting capabilities
that we didn't support before. Maybe worth mentioning in the JIRA ticket as
well.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14668,11 +14669,9 @@ private void validateCreateView()
}
throw new SemanticException(msg);
}
- if (!isValidAutomaticRewritingMaterialization()) {
- String errorMessage = "Only query text based automatic rewriting is
available for materialized view. " +
- getInvalidAutomaticRewritingMaterializationReason();
- console.printError(errorMessage);
- LOG.warn(errorMessage);
+ String validationWarningMessage =
automaticRewritingValidationResult.getErrorMessage();
Review Comment:
Why don't we check the supported algorithms here?
##########
ql/src/test/queries/clientpositive/materialized_view_rewrite_by_text_11.q:
##########
@@ -0,0 +1,13 @@
+set hive.support.concurrency=true;
+set hive.txn.manager=org.apache.hadoop.hive.ql.lockmgr.DbTxnManager;
+set hive.strict.checks.cartesian.product=false;
+set hive.materializedview.rewriting=true;
+
+create table cmv_basetable (a int, b varchar(256), c decimal(10,2))
+stored as orc TBLPROPERTIES ('transactional'='true');
+
+insert into cmv_basetable values (1, 'alfred', 10.30),(2, 'bob', 3.14),(2,
'bonnie', 172342.2),(3, 'calvin', 978.76),(3, 'charlie', 9.8);
+
+create materialized view cmv_mat_view disable rewrite as select t1.a from
cmv_basetable t1 left outer join cmv_basetable t2 on (t1.a = t1.b);
+
+alter materialized view cmv_mat_view enable rewrite;
Review Comment:
This appears as a new file although in fact it is a negative test that was
moved. Did you try to use `git mv` for performing the change so that it appears
better in git history/diff.
##########
ql/src/test/queries/clientpositive/materialized_view_rewrite_by_text_10.q:
##########
@@ -0,0 +1,11 @@
+-- Materialzed view definition has non-deterministic function
+set hive.support.concurrency=true;
+set hive.txn.manager=org.apache.hadoop.hive.ql.lockmgr.DbTxnManager;
+
+CREATE TABLE EMPS (ENAME STRING, BIRTH_EPOCH_SECS INT) STORED AS ORC
TBLPROPERTIES ('transactional'='true');
+
+CREATE MATERIALIZED VIEW v_emp AS SELECT * FROM EMPS WHERE BIRTH_EPOCH_SECS <=
UNIX_TIMESTAMP();
Review Comment:
Shouldn't the CREATE fail completely? I mentioned this in the code as well
but I don't understand why CREATE and ALTER behave differently.
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/AutomaticRewritingValidationResult.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.metadata;
+
+import org.apache.calcite.plan.RelOptMaterialization;
+
+import java.util.EnumSet;
+import java.util.Set;
+
+/**
+ * Hive extension of {@link RelOptMaterialization}.
+ */
Review Comment:
The Javadoc seems irrelevant to this class. Also I don't understand from why
the class is named "Automatic"? Is there a manual rewritting? If there is no
reason for having automatic in the name I would drop it to avoid confusion and
keep things sorter and more readable.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/materialized/alter/rewrite/AlterMaterializedViewRewriteOperation.java:
##########
@@ -64,9 +68,12 @@ public int execute() throws HiveException {
}
throw new HiveException(msg);
}
- if (!planner.isValidAutomaticRewritingMaterialization()) {
- throw new HiveException("Cannot enable rewriting for materialized
view. " +
- planner.getInvalidAutomaticRewritingMaterializationReason());
+ AutomaticRewritingValidationResult validationResult =
planner.getAutomaticRewritingValidationResult();
+ String validationErrorMessage = validationResult.getErrorMessage();
+ if (validationResult.getSupportedRewriteAlgorithms().isEmpty()) {
+ throw new HiveException(validationErrorMessage);
Review Comment:
There seems to be some inconsistency between CREATE and ALTER VIEW
statements. Here we throw an exception when there are no rewrite algorithms but
in `SemanticAnalyzer#validateCreateView` we simply log an error to the console.
--
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]
