HIVE-28620: Query result is cached in case of IOWD if the subquery is not trivial

[abstractdog]

What changes were proposed in this pull request?

Pass the root QB object to different operator tree generation paths. Behavior change is not expected because the root QB is only used in getDestinationFilePath with this patch.

Why are the changes needed?

To reach flags that were set on the root QB and apply the whole query. Otherwise, a lot of bad things can happen, details in jira.

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

Tested on cluster, unit test added.
The patch is uploaded in 2 commits (will be squashed of course), the first commit contains the unit test case which fails without the fix, which is in the second commit.

[deniskuzZ]

It seems to me that this could be done with fewer code changes, however, I could be completely wrong, but to be safe I think we should ask review from @kasakrisz
can't we pass the needed flags from the rootQB to qb?

[abstractdog]

It seems to me that this could be done with fewer code changes, however, I could be completely wrong, but to be safe I think we should ask review from @kasakrisz can't we pass the needed flags from the rootQB to qb?

I was thinking about the same
there are flags that should not change across the different levels, like the one that this patch is about: isInsertOverwriteDir
if it's set once, it applies to the whole query theoretically: there is no such thing as "some query blocks suggest that the query is IOWD, while others don't"
how to implement this properly in this SemanticAnalyzer monster...is a completely different question :D open to ideas

I believe the worst thing that's currently happening is this:

hive/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java

Line 12435 in e48cb8e

setQB(qb);

a field variable is changed while running a recursive method, with all due respect, but without knowing the context, this looks awful :(
however, I wasn't brave enough to touch it for the sake of this fix

[abstractdog]

It seems to me that this could be done with fewer code changes, however, I could be completely wrong, but to be safe I think we should ask review from @kasakrisz can't we pass the needed flags from the rootQB to qb?

I was thinking about the same there are flags that should not change across the different levels, like the one that this patch is about: isInsertOverwriteDir if it's set once, it applies to the whole query theoretically: there is no such thing as "some query blocks suggest that the query is IOWD, while others don't" how to implement this properly in this SemanticAnalyzer monster...is a completely different question :D open to ideas

I believe the worst thing that's currently happening is this:

hive/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java

Line 12435 in e48cb8e

setQB(qb);

a field variable is changed while running a recursive method, with all due respect, but without knowing the context, this looks awful :(
however, I wasn't brave enough to touch it for the sake of this fix

wait, maybe I have to revise what I said

      Operator<?> operator = genPlan(qb, qbexpr);

looks like the original qb is passed to the method, so I can reuse it, let me try :D

[abstractdog]

pushed 392c9b9
thanks for the good catch @deniskuzZ , looks like almost all the signature changes were unnecessary, except where the actual issue happened: getDestinationFilePath, where this.qb was silently used
I also changed qb param to qbParam in genPlan, it's dangerous that a param can hide the field in java, especially in this context

new test case passed with the latest change

[kasakrisz]

Passing qpParam.getQbJoinTree() is not necessary if qpParam is also passed.

[deniskuzZ]

sonar reported number of new findings

[abstractdog]

sonar reported number of new findings

checked, most of them are not introduced now, I was only able to fix one exception in the unit test

[sonarcloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[abstractdog]

is this ready for a +1 guys? :)

[deniskuzZ]

LGTM +1