Skip to content

HIVE-24167: TPC-DS query 14 fails while generating plan for the filter #5037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
okumin wants to merge 9 commits into from
Closed

HIVE-24167: TPC-DS query 14 fails while generating plan for the filter #5037

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
217b26d
HIVE-24167: TPC-DS query 14 fails while generating plan for the filter
okumin Jan 25, 2024
6ed384e
Restore the original PlanMapper#link
okumin Jan 26, 2024
dbfe700
Merge groups on linking an ASTNode with a FilterOperator
okumin Jan 26, 2024
ebc0278
Revert "Merge groups on linking an ASTNode with a FilterOperator"
okumin Jan 26, 2024
393bc43
Link two entities based on only identities and merge groups on StatsR…
okumin Jan 26, 2024
e43fe90
Update TPCDS tests
okumin Jan 26, 2024
e5c26bd
Revert "Link two entities based on only identities and merge groups o…
okumin Jan 27, 2024
e8b694f
Don't link Operators with signatures by default and merge groups on S…
okumin Jan 27, 2024
a95ebad
Replace pm.merge in StatsRulesProcFactory with pm.link
okumin Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/GroupTransformer.java
View file
Open in desktop

This file was deleted.

47 changes: 8 additions & 39 deletions ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
Expand Down Expand Up @@ -200,54 +199,30 @@ public void merge(Object o1, Object o2) {
}

private void link(Object o1, Object o2, boolean mayMerge) {

Set<Object> keySet = Collections.newSetFromMap(new IdentityHashMap<Object, Boolean>());
keySet.add(o1);
keySet.add(o2);
keySet.add(getKeyFor(o1));
keySet.add(getKeyFor(o2));

Set<EquivGroup> mGroups = Collections.newSetFromMap(new IdentityHashMap<EquivGroup, Boolean>());

for (Object object : keySet) {
EquivGroup group = objectMap.get(object);
if (group != null) {
mGroups.add(group);
}
}
if (mGroups.size() > 1) {
final EquivGroup group1 = objectMap.get(o1);
final EquivGroup group2 = objectMap.get(o2);
if (group1 != null && group2 != null && group1 != group2) {
Comment thread
okumin marked this conversation as resolved.
Outdated
if (!mayMerge) {
throw new RuntimeException("equivalence mapping violation");
Comment thread
okumin marked this conversation as resolved.
Outdated
}
EquivGroup newGrp = new EquivGroup();
newGrp.add(o1);
newGrp.add(o2);
for (EquivGroup g : mGroups) {
for (Object o : g.members) {
newGrp.add(o);
}
}
group1.members.forEach(newGrp::add);
group2.members.forEach(newGrp::add);
groups.add(newGrp);
groups.removeAll(mGroups);
groups.remove(group1);
groups.remove(group2);
} else {
EquivGroup targetGroup = mGroups.isEmpty() ? new EquivGroup() : mGroups.iterator().next();
EquivGroup targetGroup = group1 != null ? group1 : (group2 != null ? group2 : new EquivGroup());
Comment thread
okumin marked this conversation as resolved.
Outdated
groups.add(targetGroup);
targetGroup.add(o1);
targetGroup.add(o2);
}

}

private OpTreeSignatureFactory signatureCache = OpTreeSignatureFactory.newCache();

private Object getKeyFor(Object o) {
if (o instanceof Operator) {
Operator<?> operator = (Operator<?>) o;
return signatureCache.getSignature(operator);
}
return o;
}

public <T> List<T> getAll(Class<T> clazz) {
List<T> ret = new ArrayList<>();
for (EquivGroup g : groups) {
Expand All @@ -256,12 +231,6 @@ public <T> List<T> getAll(Class<T> clazz) {
return ret;
}

public void runMapper(GroupTransformer mapper) {
for (EquivGroup equivGroup : groups) {
mapper.map(equivGroup);
}
}

public <T> List<T> lookupAll(Class<T> clazz, Object key) {
EquivGroup group = objectMap.get(key);
if (group == null) {
Expand Down
27 changes: 27 additions & 0 deletions ql/src/test/queries/clientpositive/cbo_cte_materialization.q
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--! qt:dataset:src

set hive.optimize.cte.materialize.threshold=1;
set hive.optimize.cte.materialize.full.aggregate.only=false;

EXPLAIN CBO
WITH materialized_cte AS (
SELECT key, value FROM src WHERE key != '100'
),
another_materialized_cte AS (
SELECT key, value FROM src WHERE key != '100'
)
SELECT a.key, a.value, b.key, b.value
FROM materialized_cte a
JOIN another_materialized_cte b ON a.key = b.key
ORDER BY a.key;
Comment on lines +6 to +16

@zabetak zabetak Feb 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding (or replacing this with) a traditional EXPLAIN where we can see the effect of hive.optimize.cte.materialize.threshold and the full plan for the materialized ctes.


WITH materialized_cte AS (
SELECT key, value FROM src WHERE key != '100'
),
another_materialized_cte AS (
SELECT key, value FROM src WHERE key != '100'
)
SELECT a.key, a.value, b.key, b.value
FROM materialized_cte a
JOIN another_materialized_cte b ON a.key = b.key
ORDER BY a.key;
Comment on lines +18 to +27

@zabetak zabetak Feb 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is in the compilation phase so I don't think we need to actually run the query. Consider dropping the execution. If you opt to keep it then I would suggest crafting a much simpler test (without 1K lines in the output). It would be nice to keep the execution time of our test suite as low as possible. Moreover, it is not easy to see what the src table contains so verifying that the result is indeed correct is cumbersome.

1 change: 0 additions & 1 deletion ql/src/test/queries/clientpositive/perf/cbo_query14.q
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
--! qt:disabled:HIVE-24167
set hive.mapred.mode=nonstrict;
-- start query 1 in stream 0 using template query14.tpl and seed 1819994127
explain cbo
Expand Down
1 change: 0 additions & 1 deletion ql/src/test/queries/clientpositive/perf/query14.q
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
--! qt:disabled:HIVE-24167
set hive.mapred.mode=nonstrict;
-- start query 1 in stream 0 using template query14.tpl and seed 1819994127
explain
Expand Down
Loading