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

okumin · 2024-01-25T08:36:29Z

What changes were proposed in this pull request?

Make it possible for PlanMapper to link related Operators or RelNodes correctly.

For your reference, I made a Gist page explaining the behaviors of PlanMapper and alternative approaches in my mind.
https://gist.github.com/okumin/b111fe0a911507bdf6a7204f49b9cb72

See also the PR where I implemented another option, "Combine RelNodes with RelTreeSignature".

Why are the changes needed?

No. 14 of TPC-DS or other complex queries with CTE materialized fail due to this bug.
https://issues.apache.org/jira/browse/HIVE-24167

Does this PR introduce any user-facing change?

No. This is a bug fix.

Is the change a dependency upgrade?

No.

How was this patch tested?

I added a new test case, which fails with the current master branch, and restored No. 14 of TPC-DS.

aturoczy

Minimal change request, and comment

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

This reverts commit dbfe700.

…ulesProcFactory

…n StatsRulesProcFactory" This reverts commit 393bc43.

…tatsRulesProcFactory

sonarcloud · 2024-01-29T14:14:53Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

zabetak · 2024-02-02T09:20:03Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

    }

+    if (!mayMerge) {
+      throw new RuntimeException(String.format(
+          "Failed to link %s and %s. This error mostly means a bug of Hive",


nit: An uncaught RuntimeException almost always denotes a bug of the application. I don't think we need to make the exception that verbose stating the obvious.

Adding the operators which led to exception in the message is good idea, makes sense.

I find this error message pretty cloudy on what the issue is...

I think the sentecte "Equivalence mapping violation" should be there - as that's what the problem is...we have 2 groups; not allowed to put them into the same equiv group -> big trouble...

zabetak · 2024-02-02T09:44:09Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+    registerSignature(o1);
+    registerSignature(o2);


Linking operators implicitly (using signatures) was added by @kgyrtkirk in HIVE-18926. This change reverts that logic and takes signatures out of the equation for determining equivalent groups.

My understanding is that if this change goes in operators can be linked with signatures only explicitly. I don't see test failures and .q.out changes due to this so it seems that the change does not have a big impact on existing use-cases. Moreover, it fixes some known compilation crashes so I consider this an improvement over the current situation.

I don't see a significant risk in merging this change but I will let @kgyrtkirk comment in case I am missing something.

Assuming that we do not allow implicit linking via signatures I don't know what's the point of registering the signature at this stage. My understanding is that the cache is meant to improve performance not a means to perform equivalence checks. If we don't make use of the signature here then I don't think we should compute it. @okumin although you added some comments justifying the registration I would appreciate some additional clarifications regarding these additions.

This change reverts that logic and takes signatures out of the equation for determining equivalent groups.

haven't read all of it - but if thats true - please don't do that...
looking at the volume of changes - it must be changing something fundamental...

what's the issue - is it an equiv violation? can you put the stacktrace somewhere?
I think that should be fixed without altering this part - it might be a missing link; or a missing signature annotation somewhere - you should not change this at all...

all these things could enable to back-map runtime statistict up to the calcite join planning phase...but I think that feature was never completed/enabled

to get a failing test for these changes you would need 2 ops which have conflicting signatures stored into the metastore ; loaded back and they might get applied incorrectly...
possibly pretty hard to do...

Assuming that we do not allow implicit linking via signatures I don't know what's the point of registering the signature at this stage

That's because some optimization can mutate Operators, changing their true signatures. One example I remember is TableScanPPD. I'm not 100% sure which signatures should be used in that case, but it sounds more reasonable and predictable for me to use ones before optimization than ones in the middle of optimization. I would say Operators should not be mutable for the signing purpose, but it is a too fundamental change.

I will make a reply to @kgyrtkirk in another thread as I guess those comments were written before reading my gist.

I missed mentioning what are affected by such optimizations. Many test cases on the following revision failed with equivalence mapping violation.
217b26d

That's because it tries to LINK a pre-optimized operator with a post-optimized operator.

Before optimization

Operator A has a signature SA and is grouped as GA

Operator B has a signature SB and is grouped as GB

AuxSignatureLinker doesn't MERGE GA and GB as A and B have different signatures. Note that non aux signatures are not cached here

After optimization

Operator B is optimized, and its signature becomes SA

StatsRulePsrocFactory tries to LINK GA and GB as A and B share the same signature SA -> equivalence mapping violation

Potentially, GA and GB are actually mergeable though the current implementation doesn't do so.

zabetak · 2024-02-02T10:06:44Z

ql/src/test/queries/clientpositive/cbo_cte_materialization.q

+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;


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.

zabetak · 2024-02-02T10:09:28Z

ql/src/test/queries/clientpositive/cbo_cte_materialization.q

+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;


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.

kgyrtkirk · 2024-02-02T11:27:50Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+    registerSignature(o1);
+    registerSignature(o2);


This change reverts that logic and takes signatures out of the equation for determining equivalent groups.

haven't read all of it - but if thats true - please don't do that...
looking at the volume of changes - it must be changing something fundamental...

what's the issue - is it an equiv violation? can you put the stacktrace somewhere?
I think that should be fixed without altering this part - it might be a missing link; or a missing signature annotation somewhere - you should not change this at all...

all these things could enable to back-map runtime statistict up to the calcite join planning phase...but I think that feature was never completed/enabled

kgyrtkirk · 2024-02-02T11:28:49Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+  private void registerSignature(Object o) {
+    if (o instanceof Operator) {
+      getSignatureOf((Operator<?>) o);
+    }
  }


whats the point of this method? the cache will compute it if needed...what the point of pre-registering - if that's needed its not a cache anymore!

kgyrtkirk · 2024-02-02T11:36:16Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+    // Caches signatures on the first access. A signature of an Operator could change as optimizers could mutate it,
+    // keeping its semantics. The current implementation caches the signature before optimizations so that we can
+    // link Operators with their signatures at consistent timing.


please remove this comment and don't pre-heat caches unless it gives real benefits
the purpose of the cache is not something like this ; but to prevent O(N^2) computation ( and also to reduce storage size by making it more serialize friendly...)

kgyrtkirk · 2024-02-02T11:37:21Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+    registerSignature(o1);
+    registerSignature(o2);
+    final EquivGroup linkedGroup1 = objectMap.get(o1);
+    final EquivGroup linkedGroup2 = objectMap.get(o2);


these changes will weaken this stuff - so that 2 OPs which have the same signature will be allowed -> that's not good....
its not causing much issues yet - because almost all existing operators have good signatures; but if that degrades...runtime stats will be applied incorrectly

kgyrtkirk · 2024-02-02T11:39:04Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

-      groups.add(targetGroup);
-      targetGroup.add(o1);
-      targetGroup.add(o2);
+    if (linkedGroup1 == null || linkedGroup2 == null || linkedGroup1 == linkedGroup2) {


what's all these changes do better than the old?

kgyrtkirk · 2024-02-02T11:40:54Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

    }

+    if (!mayMerge) {
+      throw new RuntimeException(String.format(
+          "Failed to link %s and %s. This error mostly means a bug of Hive",


I find this error message pretty cloudy on what the issue is...

I think the sentecte "Equivalence mapping violation" should be there - as that's what the problem is...we have 2 groups; not allowed to put them into the same equiv group -> big trouble...

kgyrtkirk · 2024-02-02T11:44:46Z

ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java

+    registerSignature(o1);
+    registerSignature(o2);


to get a failing test for these changes you would need 2 ops which have conflicting signatures stored into the metastore ; loaded back and they might get applied incorrectly...
possibly pretty hard to do...

kgyrtkirk · 2024-02-02T11:59:52Z

just seen the details you linked to the PR.

I believe the link should be allowed - but in this the merge should be allowed due to the fact that these things represent the same thing.
I see now why the signature become an obstacle - I believe the SWO was able to match the same plan-parts to only scan the table once; if you don't have link calls there to explain the situation -> that could cause equiv violations here...could you please check that?

okumin · 2024-02-02T15:27:12Z

@kgyrtkirk

I believe the link should be allowed - but in this the merge should be allowed due to the fact that these things represent the same thing.

I guess this mentions the link on creating a FilterOperator?

I see now why the signature become an obstacle - I believe the SWO was able to match the same plan-parts to only scan the table once; if you don't have link calls there to explain the situation -> that could cause equiv violations here...could you please check that?

I guess the SWO stands for SharedWorkOptimizer. With CTE materialization, each CTE is compiled separately. It means materialized_cte, another_materialized_cte, and the body part are compiled one by one in a dedicated manner. So, SharedWorkOptimizer has no chance to unify TableScanOperators located in multiple CTEs.

Just for a note. We use CTE because we need to support some cases which SharedWorkOptimizer doesn't support. An example is complex CTE + UNION ALL. We may replace CTE materialization with SharedWorkOptimizer when it becomes more mature.

kgyrtkirk · 2024-02-05T10:18:24Z

each CTE is compiled separately

got it now - I was thinking its the other way around

I was thinking about this a bit in the last couple days; here is a summary of my thoughts:

to have this feature cause trouble the following is needed:
- a failing query
- an incorrectly taken stat entry
- the bad stat entry causes further issues
- ....not very likely :D
keeping multiple equiv groups which should be merged is not fortunate - but may not cause trouble
at the end of the day all a Map<Signature, RuntimeStat> will be created from all these details
- if there is are 2 groups which have the same Signature that will mean there will be multiple candidates for the key in the map => which means those should have been in a single group...that's why I didn't really like to relax the check on the signature
  - I believe lookup-by-signatue will either return one a representative group ; even when there are more

from the listed approaches in the doc:

I liked the best the RelTreeSignature related; I think that would be the best if possioble
- this might be a way to achieve a merge between the 2 groups
- have you tried somehow relax the alias name of HiveTableScan in the RelTreeSignature ?
  - that class creates the signature of the relnodes 1-by-1 ; so you could access/customize/etc based on the type
    - possibly use the @Signature annotation
    - use something to filter out poisonous stuff @IgnoreSig or something
    - by-hand stuff
on the other hand after thinking it thru what might be the outcomes - I find that the idea to not pull the signatures during linking might actually work correctly...

note: you could reachable on the ASF slack as well - we can talk on #hive as well

okumin · 2024-02-11T13:09:28Z

@kgyrtkirk
I tried the option using RelTreeSignature in the following Pull Request. I still have some concerns, but it will likely work from my perspective.
#5077

Could you please take a look and see if it seems feasible? If we prefer #5077 , I will void this PR and try to get it merged.

okumin · 2024-04-08T12:24:14Z

Let me close this one because we will likely go with #5077 or another approach.

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

217b26d

asf-ci-hive added tests pending tests failed and removed tests pending labels Jan 25, 2024

aturoczy suggested changes Jan 25, 2024

View reviewed changes

okumin added 2 commits January 26, 2024 10:41

Restore the original PlanMapper#link

6ed384e

Merge groups on linking an ASTNode with a FilterOperator

dbfe700

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending labels Jan 26, 2024

okumin added 3 commits January 26, 2024 16:16

Revert "Merge groups on linking an ASTNode with a FilterOperator"

ebc0278

This reverts commit dbfe700.

Link two entities based on only identities and merge groups on StatsR…

393bc43

…ulesProcFactory

Update TPCDS tests

e43fe90

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Jan 26, 2024

Revert "Link two entities based on only identities and merge groups o…

e5c26bd

…n StatsRulesProcFactory" This reverts commit 393bc43.

asf-ci-hive added tests pending and removed tests unstable labels Jan 27, 2024

okumin force-pushed the HIVE-24167-cbo-cte branch from 666e270 to 0430662 Compare January 27, 2024 05:29

asf-ci-hive added tests failed tests pending and removed tests pending tests failed labels Jan 27, 2024

okumin force-pushed the HIVE-24167-cbo-cte branch from 0430662 to 714bdf8 Compare January 27, 2024 05:54

Don't link Operators with signatures by default and merge groups on S…

e8b694f

…tatsRulesProcFactory

okumin force-pushed the HIVE-24167-cbo-cte branch from 714bdf8 to e8b694f Compare January 27, 2024 06:33

asf-ci-hive added tests pending tests passed and removed tests failed tests pending labels Jan 27, 2024

Replace pm.merge in StatsRulesProcFactory with pm.link

a95ebad

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending tests unstable labels Jan 29, 2024

asf-ci-hive added tests passed and removed tests pending labels Jan 29, 2024

okumin changed the title ~~[WIP] HIVE-24167: TPC-DS query 14 fails while generating plan for the filter~~ HIVE-24167: TPC-DS query 14 fails while generating plan for the filter Jan 30, 2024

okumin marked this pull request as ready for review January 30, 2024 03:27

zabetak reviewed Feb 2, 2024

View reviewed changes

kgyrtkirk reviewed Feb 2, 2024

View reviewed changes

okumin mentioned this pull request Feb 10, 2024

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

Closed

okumin closed this Apr 8, 2024

okumin deleted the HIVE-24167-cbo-cte branch April 8, 2024 12:24

okumin mentioned this pull request Sep 18, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

okumin commented Jan 25, 2024 •

edited

Loading

aturoczy left a comment

sonarcloud bot commented Jan 29, 2024

zabetak Feb 2, 2024

kgyrtkirk Feb 2, 2024

zabetak Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

okumin Feb 2, 2024

okumin Feb 2, 2024

zabetak Feb 2, 2024

zabetak Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk Feb 2, 2024

kgyrtkirk commented Feb 2, 2024

okumin commented Feb 2, 2024 •

edited

Loading

kgyrtkirk commented Feb 5, 2024 •

edited

Loading

okumin commented Feb 11, 2024

okumin commented Apr 8, 2024

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

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

Conversation

okumin commented Jan 25, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

aturoczy left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 29, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgyrtkirk commented Feb 2, 2024

okumin commented Feb 2, 2024 • edited Loading

kgyrtkirk commented Feb 5, 2024 • edited Loading

okumin commented Feb 11, 2024

okumin commented Apr 8, 2024

okumin commented Jan 25, 2024 •

edited

Loading

okumin commented Feb 2, 2024 •

edited

Loading

kgyrtkirk commented Feb 5, 2024 •

edited

Loading