[FLINK-28096][hive] Hive dialect support set variables #20000

luoyuxia · 2022-06-17T05:21:03Z

What is the purpose of the change

To make Hive dialect support set variables.

Brief change log

When it's for set command, extract the key and value, and then set the value according what kind of variable it belongs.

Verifying this change

UT

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? N/A

flinkbot · 2022-06-17T05:27:17Z

CI report:

2bec115 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

fsk119

Thanks for your contribution. I left some comments.

...rc/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveParserSetProcessor.java

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

...s/flink-connector-hive/src/test/java/org/apache/flink/connectors/hive/HiveDialectITCase.java

fsk119 · 2022-07-20T08:37:33Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

+            if (!key.equals("silent")) {
+                HiveParserSetProcessor.setVariable(hiveConf, hiveVariables, key, value);
+            }
+            return new NopOperation();


We'd better to also set the config into table config because users may set some optimization options here.

fsk119 · 2022-07-22T03:54:49Z

...hive/src/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveSetProcessor.java

+import static org.apache.hadoop.hive.conf.SystemVariables.METACONF_PREFIX;
+import static org.apache.hadoop.hive.conf.SystemVariables.SYSTEM_PREFIX;
+
+/** Counterpart of hive's {@link org.apache.hadoop.hive.ql.processors.SetProcessor}. */


Sorry. I made a mistake. It's better use {@link SetProcessor}

wuchong · 2022-07-22T08:39:03Z

The compile phase is failed.

fsk119

Thanks for update. I left some comments. It looks good in general.

fsk119 · 2022-07-25T13:05:07Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

@@ -82,6 +88,7 @@ public class HiveParser extends ParserImpl {
    private static final Method getCurrentTSMethod =
            HiveReflectionUtils.tryGetMethod(
                    SessionState.class, "getQueryCurrentTimestamp", new Class[0]);
+    private static final String HIVE_VARIABLE_PREFIX = "__hive.variable__";


It's better to move to HiveInternalOptions and describe this in the descriptions.

I think it's better to use __hive.variables__? WDYT?

fsk119 · 2022-07-25T13:08:55Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

+                        processSetCmd(
+                                hiveConf, statement.substring(commandTokens[0].length()).trim()));
+            } else {
+                throw new UnsupportedOperationException();


nit: It's better to tell users what is unsupported.

fsk119 · 2022-07-25T13:26:58Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

+            if (part[0].equals("silent")) {
+                throw new UnsupportedOperationException("Unsupported command 'set silent'.");
+            }
+            HiveSetProcessor.setVariable(hiveConf, tableConfig, hiveVariables, part[0], part[1]);


It better to let SessionContext to do the set.

fsk119 · 2022-07-25T13:28:04Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

+        String option = HiveSetProcessor.getVariable(hiveConf, hiveVariables, nwcmd);
+        // for the variable
+        throw new UnsupportedOperationException("Unsupported SET command which misses '='.");


Do we need this? I think we can remove it.

fsk119 · 2022-07-25T13:29:58Z

...-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParser.java

+            String options =
+                    HiveSetProcessor.dumpOptions(
+                            hiveConf.getChangedProperties(), hiveConf, hiveVariables);


remove if we don't support this.

fsk119 · 2022-07-25T13:36:04Z

...hive/src/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveSetProcessor.java

+        hiveConf.verifyAndSet(key, value);
+    }
+
+    public static String getVariable(


Do you want to support to show variables using SET? If not, I think we can add this when needs.

fsk119 · 2022-07-25T13:50:11Z

...s/flink-connector-hive/src/test/java/org/apache/flink/connectors/hive/HiveDialectITCase.java

+        assertThat(hiveCatalog.getHiveConf().get("yyy")).isEqualTo("5");
+        // test set hivevar:
+        tableEnv.executeSql("set hivevar:a=1");
+        tableEnv.executeSql("set hiveconf:zzz=${hivevar:a}");


I think we should renew a sql parser to test whether the test config memorizes hive-vars.

For example,

tableEnv.getConfig().setSqlDialect(SqlDialect.DEFAULT); tableEnv.executeSql("show tables"); tableEnv.getConfig().setSqlDialect(SqlDialect.HIVE);

wuchong · 2022-07-30T15:31:27Z

...s/flink-connector-hive/src/test/java/org/apache/flink/connectors/hive/HiveDialectITCase.java

+        tableEnv.getConfig().setSqlDialect(SqlDialect.DEFAULT);
+        tableEnv.executeSql("show tables");
+        tableEnv.getConfig().setSqlDialect(SqlDialect.HIVE);
+        tableEnv.executeSql("set hiveconf:zzz1=${hivevar:a}");


Use set statement to switch dialect?

Set a flink config under hive dialect and check it works.

1: We maynot use set statement to switch dialect directly for the SetOperation is executed in SqlClient.
2: The Flink's SetOperation will be executed in SqlClient , so we can't set it in tableEnv. But the test set.q in flink-sql-client has covered the case that set flink config in HiveDialect.

wuchong

LGTM.

Do you have other concerns? @fsk119

flinkbot added the component=Connectors/Hive label Jun 17, 2022

fsk119 reviewed Jul 20, 2022

View reviewed changes

luoyuxia force-pushed the FLINK-28096-1 branch from 18202b0 to 0a44c79 Compare July 21, 2022 01:23

fsk119 reviewed Jul 22, 2022

View reviewed changes

luoyuxia force-pushed the FLINK-28096-1 branch 3 times, most recently from 95d6432 to 432037b Compare July 24, 2022 08:13

fsk119 reviewed Jul 25, 2022

View reviewed changes

luoyuxia force-pushed the FLINK-28096-1 branch 3 times, most recently from 79fbc1e to cbb3ce2 Compare July 27, 2022 07:02

wuchong reviewed Jul 30, 2022

View reviewed changes

luoyuxia added 3 commits July 31, 2022 10:26

[FLINK-28096][hive] Hive dialect support set variable

508e7d0

[FLINK-28096][hive] make execute SetOperation in Hive's way

4f56be6

fix merge conflict and refine test

2bec115

luoyuxia force-pushed the FLINK-28096-1 branch from cbb3ce2 to 2bec115 Compare July 31, 2022 05:46

wuchong approved these changes Aug 1, 2022

View reviewed changes

wuchong merged commit a6a7063 into apache:master Aug 1, 2022

huangxiaofeng10047 pushed a commit to huangxiaofeng10047/flink that referenced this pull request Nov 3, 2022

[FLINK-28096][hive] Hive dialect support set variables (apache#20000)

701c5d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-28096][hive] Hive dialect support set variables #20000

[FLINK-28096][hive] Hive dialect support set variables #20000

luoyuxia commented Jun 17, 2022

flinkbot commented Jun 17, 2022 •

edited

fsk119 left a comment

fsk119 Jul 20, 2022

fsk119 Jul 22, 2022

wuchong commented Jul 22, 2022

fsk119 left a comment

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

fsk119 Jul 25, 2022

wuchong Jul 30, 2022

luoyuxia Jul 31, 2022

wuchong left a comment

[FLINK-28096][hive] Hive dialect support set variables #20000

[FLINK-28096][hive] Hive dialect support set variables #20000

Conversation

luoyuxia commented Jun 17, 2022

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Jun 17, 2022 • edited

CI report:

fsk119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuchong commented Jul 22, 2022

fsk119 left a 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

wuchong left a comment

Choose a reason for hiding this comment

flinkbot commented Jun 17, 2022 •

edited