You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "kasakrisz (via GitHub)" <gi...@apache.org> on 2023/12/15 12:22:36 UTC
[PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
kasakrisz opened a new pull request, #4951:
URL: https://github.com/apache/hive/pull/4951
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### Is the change a dependency upgrade?
<!--
If yes, please attach a file with output from mvn dependency:tree to validate a complete upgrade of dependency.
If no, write 'No'.
-->
### How was this patch tested?
```
mvn test -Dtest.output.overwrite -DskipSparkTests -Dtest=TestMiniLlapLocalCliDriver -Dqfile=materialized_view_no_cbo_rewrite.q,materialized_view_rewrite_by_text_5.q,materialized_view_create_rewrite_6.q,materialized_view_rewrite_3.q,materialized_view_rewrite_by_text_10.q -pl itests/qtest -Pitests
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz merged PR #4951:
URL: https://github.com/apache/hive/pull/4951
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434213876
##########
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:
There are two types of failures:
1. disable both rewrite algs.
2. allow text base only
If type 1) is found we can stop traversing the tree.
If type 2) is found we record it if this is the first one from this type but we have to continue traversing because there might be a type 1).
Actually we print only one failure and if there are more than one from type 2) it doesn't matter which one is printed. I kept the logic avoid too many golden file changes.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434243023
##########
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:
Renamed and disable rewrite if no rewrite alg is available.
##########
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:
done
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1867617807
@kasakrisz By the way the test failure is relevant so make sure you address it before merging.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434113870
##########
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:
Actually thinking a bit more on this, it may be fine to allow the creation of the materialized view as long as it does not create issues with query rewriting and (incremental) refresh. I was thinking of use-cases where users CREATE views and refresh them manually (let's say once every month) so in that case UNIX_TIMESTAMP and other similar CURRENT_ functions may make sense.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1857901876
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[20 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1862462058
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[7 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434243682
##########
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:
Removed javadoc and rename the class to MaterializationValidationResult
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1880686346
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[13 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434244790
##########
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:
Changed that create mv not just log the error but also disables rewrite for MV.
##########
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:
done
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434249869
##########
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:
I tried it. Let's see the result :)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1866683995
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[13 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434247922
##########
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:
Yes. Creation should not fail but a message is logged and MV is disabled for rewrite if none of the algorithms are available.
In case of Alter the goal is to enable rewrite but if none of the algorithms are available I think it is ok to fail.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434973891
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -15930,18 +15932,14 @@ public ColsAndTypes(String cols, String colTypes) {
public String colTypes;
}
- public String getInvalidAutomaticRewritingMaterializationReason() {
- return invalidAutomaticRewritingMaterializationReason;
- }
-
- public void setInvalidAutomaticRewritingMaterializationReason(
- String invalidAutomaticRewritingMaterializationReason) {
- this.invalidAutomaticRewritingMaterializationReason =
- invalidAutomaticRewritingMaterializationReason;
+ public MaterializationValidationResult getAutomaticRewritingValidationResult() {
+ return materializationValidationResult;
}
- public boolean isValidAutomaticRewritingMaterialization() {
- return (invalidAutomaticRewritingMaterializationReason == null);
+ public void setAutomaticRewritingValidationResult(
+ MaterializationValidationResult materializationValidationResult) {
+ this.materializationValidationResult =
+ materializationValidationResult;
Review Comment:
nit: Since we dropped "Automatic" from the result we can as well drop it from the methods since we are renaming them anyways.
Feel free to disregard or address directly during merge without waiting for a new CI run.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4951:
URL: https://github.com/apache/hive/pull/4951#issuecomment-1860467462
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[23 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4951&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4951)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434269899
##########
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:
Actually these capabilities already exists if rewrite is not disabled at creation time. However Alter doesn't allow these. This patch tries to improve Alter too.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1444287379
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -15930,18 +15932,14 @@ public ColsAndTypes(String cols, String colTypes) {
public String colTypes;
}
- public String getInvalidAutomaticRewritingMaterializationReason() {
- return invalidAutomaticRewritingMaterializationReason;
- }
-
- public void setInvalidAutomaticRewritingMaterializationReason(
- String invalidAutomaticRewritingMaterializationReason) {
- this.invalidAutomaticRewritingMaterializationReason =
- invalidAutomaticRewritingMaterializationReason;
+ public MaterializationValidationResult getAutomaticRewritingValidationResult() {
+ return materializationValidationResult;
}
- public boolean isValidAutomaticRewritingMaterialization() {
- return (invalidAutomaticRewritingMaterializationReason == null);
+ public void setAutomaticRewritingValidationResult(
+ MaterializationValidationResult materializationValidationResult) {
+ this.materializationValidationResult =
+ materializationValidationResult;
Review Comment:
Renamed.
There were test failures in the last CI result due to a deprecated q out file. I have to rerun it anyway.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27948: Wrong results when using materialized views with non-deterministic/dynamic functions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4951:
URL: https://github.com/apache/hive/pull/4951#discussion_r1434113870
##########
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:
Actually thinking a bit more on this, it may be fine to allow the creation of the materialized view as long as it does not create issues with query rewriting and (incremental) refresh.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org