HIVE-28409: Column lineage when creating view is missing if atlas HiveHook is set #5370

kasakrisz · 2024-07-26T12:33:55Z

What changes were proposed in this pull request?

Remove hardcoded Atlas HiveHook related filters from lineage information Generator and introduce an independent setting to filter based on statement/operation type.
Add isView property to QueryProperties to indicate that this statement is a logical view creation.

Why are the changes needed?

Lineage information was not generated in case of create view operation. See jira for details: https://issues.apache.org/jira/browse/HIVE-28409

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

mvn test -Dtest.output.overwrite -DskipSparkTests -Dtest=TestMiniLlapLocalCliDriver -Dqfile=lineage6.q -pl itests/qtest -Pitests
mvn test -Dtest=TestGenerator.java -pl ql

InvisibleProgrammer · 2024-07-26T14:45:00Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -3865,6 +3865,8 @@ public static enum ConfVars {
        "get old behavior, if desired. See, test-case in patch for HIVE-6689."),
    HIVE_LINEAGE_INFO("hive.lineage.hook.info.enabled", false,
        "Whether Hive provides lineage information to hooks."),
+    HIVE_LINEAGE_STATEMENT_FILTER("hive.lineage.statement.filter", "All",


I don't know if we have a rule about it or not. But can the default value be lowercase?
I would love to avoid future confusion about if is that All, all or ALL.

Also, could you please put some description and and example values there that can help us to understand what that config value exactly does?

The comparison is not case sensitive to make this more felxible

if (ALL.equalsIgnoreCase(valueText)) {

if (NONE.equalsIgnoreCase(valueText)) {

HiveOperation enumValue = EnumUtils.getEnumIgnoreCase(HiveOperation.class, valueText);

I added an upper case default value because other values coming from HiveOperation are in upper case. There are other configs with upper case values so I think it is not an exception.
I also added the missing description.

InvisibleProgrammer · 2024-07-26T14:45:14Z

ql/src/java/org/apache/hadoop/hive/ql/hooks/LineageLogger.java

@@ -6,7 +6,7 @@
 * 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
- *
+ *f


accident typo

InvisibleProgrammer · 2024-07-26T14:47:13Z

ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java

@@ -79,7 +79,7 @@ public void initialize(HiveConf hiveConf) {
        || postExecHooks.contains("org.apache.hadoop.hive.ql.hooks.PostExecutePrinter")
        || postExecHooks.contains("org.apache.hadoop.hive.ql.hooks.LineageLogger")
        || postExecHooks.contains("org.apache.atlas.hive.hook.HiveHook")) {
-      transformations.add(new Generator(postExecHooks));
+      transformations.add(Generator.fromConf(hiveConf));


I assume that before that change, we read the transformations from the postExecHooks that is as far I remember, calculated once. And from now, we read those again at every single query execution.
Am I correct?

postExecHooks also calculated at every query execution:

hive/ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java

Lines 75 to 77 in 09553fc

Set<String> postExecHooks = Sets.newHashSet(

Splitter.on(",").trimResults().omitEmptyStrings().split(

Strings.nullToEmpty(HiveConf.getVar(hiveConf, HiveConf.ConfVars.POST_EXEC_HOOKS))));

I think this should be addressed in a follow-up only if this is bottleneck.

InvisibleProgrammer · 2024-07-26T14:59:16Z

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java

+
+      HiveOperation enumValue = EnumUtils.getEnumIgnoreCase(HiveOperation.class, valueText);
+      if (enumValue == null) {
+        throw new EnumConstantNotPresentException(HiveOperation.class, valueText);


I'm not familiar with that part: what will be the user experience if they add this value to the conf: "Hello" ?

Exception is thrown at compile time.
Example:

java.lang.EnumConstantNotPresentException: org.apache.hadoop.hive.ql.plan.HiveOperation.Hello at org.apache.hadoop.hive.ql.optimizer.lineage.Generator.createFilterPredicateFromConf(Generator.java:106) at org.apache.hadoop.hive.ql.optimizer.lineage.Generator.fromConf(Generator.java:72) at org.apache.hadoop.hive.ql.optimizer.Optimizer.initialize(Optimizer.java:82) at org.apache.hadoop.hive.ql.parse.SemanticAnalyzer.analyzeInternal(SemanticAnalyzer.java:13274) ...

InvisibleProgrammer · 2024-07-26T15:10:30Z

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java

+        operations.stream().anyMatch(hiveOperation -> filterMap.get(hiveOperation).apply(parseContext));
+  }
+
+  private final Predicate<ParseContext> statementFilter;


Can you move that declaration and the constructor up so that the class can keep that pattern: fields -> constructors -> methods?

Everything before statementFilter is static. Where should those static fields and methods go?

I would really not add method declarations before field ones.
What about static fields, instance fields, static initialization block, constructor, public methods, private methods?

I usually move static members to the beginning of the file but I don't have strong opinion about this.
I changed the order as you suggested.

InvisibleProgrammer · 2024-07-26T15:22:54Z

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java

-        LOG.debug("Not evaluating lineage");
-        return pctx;
-      }
+    if (!statementFilter.test(pctx)) {


I have a fear that this change will break existing configs: Previously, we said that if the Atlas HiveHook is set, we will run that transformation. Now we are saying that if hive.lineage.statement.filter is set, we do the transformation.

Yes, it breaks existing configs but previously Atlas HiveHook-related filters were hardcoded and impossible to test. Now hive.lineage.statement.filter is independent of hooks and hence applies for any hook making it possible to test using LineageLogger.

InvisibleProgrammer · 2024-08-02T04:11:30Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

-        "Whether Hive provides lineage information to hooks."),
+        "Whether Hive provides lineage information to hooks." +
+            "Deprecated: use hive.lineage.statement.filter instead."),
+    HIVE_LINEAGE_STATEMENT_FILTER("hive.lineage.statement.filter", "ALL",


I would suggest using NONE as default.

NONE means that this functionality is never triggered. IMHO all functionality should be tested otherwise follow-up changes in the project may make it impossible to turn it on in the future. We have experienced this in the past a few times.

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java

deniskuzZ

LGTM +1

…eHook is set

deniskuzZ · 2024-08-23T11:07:06Z

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java

+    LOAD(parseContext -> !(parseContext.getLoadTableWork() == null || parseContext.getLoadTableWork().isEmpty())),
+    QUERY(parseContext -> parseContext.getQueryProperties().isQuery()),
+    ALL(parseContext -> true),
+    NONE(parseContext -> false);


i still think that NONE is redundant, but I am fine to keep it

sonarqubecloud · 2024-08-23T11:20:59Z

Quality Gate passed

Issues
9 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 SonarCloud

kasakrisz marked this pull request as draft July 26, 2024 12:34

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

InvisibleProgrammer reviewed Jul 26, 2024

View reviewed changes

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jul 29, 2024

kasakrisz force-pushed the HIVE-28409-master-atlas-lineage branch from 81c89e9 to 35a9c6a Compare July 29, 2024 13:44

asf-ci-hive added tests pending and removed tests failed labels Jul 29, 2024

kasakrisz marked this pull request as ready for review July 29, 2024 13:45

asf-ci-hive added tests failed and removed tests pending labels Jul 29, 2024

kasakrisz force-pushed the HIVE-28409-master-atlas-lineage branch from 35a9c6a to 4fbab8e Compare July 30, 2024 04:37

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jul 30, 2024

InvisibleProgrammer reviewed Aug 2, 2024

View reviewed changes

kasakrisz force-pushed the HIVE-28409-master-atlas-lineage branch from 0e66ae2 to b8e80c7 Compare August 3, 2024 06:48

asf-ci-hive added tests pending tests passed and removed tests passed tests pending labels Aug 3, 2024

deniskuzZ reviewed Aug 8, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Aug 8, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java Show resolved Hide resolved

deniskuzZ reviewed Aug 8, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Aug 8, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/optimizer/lineage/Generator.java Show resolved Hide resolved

kasakrisz requested review from deniskuzZ and InvisibleProgrammer August 21, 2024 13:19

deniskuzZ approved these changes Aug 23, 2024

View reviewed changes

kasakrisz and others added 8 commits August 23, 2024 11:52

HIVE-28409: Column lineage when creating view is missing if atlas Hiv…

1d14221

…eHook is set

revert typo

75787b6

add description

e30d4e5

rat

76a0e26

use EnumMap

5a91e20

restore fetch task conversion for create view

01cbcd0

reorder members

9ac7895

introduce new enum instead of HiveOperation

346f7ab

kasakrisz force-pushed the HIVE-28409-master-atlas-lineage branch from b8e80c7 to 346f7ab Compare August 23, 2024 10:21

asf-ci-hive added tests pending and removed tests passed labels Aug 23, 2024

deniskuzZ reviewed Aug 23, 2024

View reviewed changes

asf-ci-hive added tests passed and removed tests pending labels Aug 23, 2024

kasakrisz merged commit d3bbb03 into apache:master Aug 26, 2024
6 checks passed

kasakrisz deleted the HIVE-28409-master-atlas-lineage branch August 26, 2024 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-28409: Column lineage when creating view is missing if atlas HiveHook is set #5370

HIVE-28409: Column lineage when creating view is missing if atlas HiveHook is set #5370

kasakrisz commented Jul 26, 2024 •

edited

Loading

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Aug 2, 2024

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Aug 2, 2024

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Aug 2, 2024

kasakrisz Aug 3, 2024 •

edited

Loading

InvisibleProgrammer Jul 26, 2024

kasakrisz Jul 29, 2024

InvisibleProgrammer Aug 2, 2024

kasakrisz Aug 3, 2024

deniskuzZ left a comment

deniskuzZ Aug 23, 2024 •

edited

Loading

sonarqubecloud bot commented Aug 23, 2024

	Set<String> postExecHooks = Sets.newHashSet(
	Splitter.on(",").trimResults().omitEmptyStrings().split(
	Strings.nullToEmpty(HiveConf.getVar(hiveConf, HiveConf.ConfVars.POST_EXEC_HOOKS))));

HIVE-28409: Column lineage when creating view is missing if atlas HiveHook is set #5370

HIVE-28409: Column lineage when creating view is missing if atlas HiveHook is set #5370

Conversation

kasakrisz commented Jul 26, 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?

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

kasakrisz Aug 3, 2024 • edited Loading

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

deniskuzZ left a comment

Choose a reason for hiding this comment

deniskuzZ Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 23, 2024

Quality Gate passed

kasakrisz commented Jul 26, 2024 •

edited

Loading

kasakrisz Aug 3, 2024 •

edited

Loading

deniskuzZ Aug 23, 2024 •

edited

Loading