[FLINK-13814][hive] HiveTableSink should strip quotes from partition … #9502

lirui-apache · 2019-08-22T07:01:21Z

…values

What is the purpose of the change

Strip quotes from partition value in order to get proper string values.

Brief change log

Strip quotes when we get partition spec from the planner
Added test to verify string partition values are properly handled
Migrated some existing tests to TableEnvHiveConnectorTest since the planner has now supported PartitionableTableSink

Verifying this change

Added test case

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, Yarn/Mesos, 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? NA

flinkbot · 2019-08-22T07:03:43Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 6901271 (Fri Aug 30 22:22:15 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-08-22T07:05:55Z

CI report:

2227892 : SUCCESS Build
5ad14b3 : SUCCESS Build
2215b05 : SUCCESS Build
6901271 : SUCCESS Build

lirui-apache · 2019-08-22T07:09:55Z

cc @xuefuz @bowenli86 @zjuwangg

xuefuz · 2019-08-22T18:19:51Z

...ctors/flink-connector-hive/src/main/java/org/apache/flink/connectors/hive/HiveTableSink.java

@@ -170,7 +171,7 @@ public void setStaticPartition(Map<String, String> partitionSpec) {
 		staticPartitionSpec = new LinkedHashMap<>();
 		for (String partitionCol : getPartitionFieldNames()) {
 			if (partitionSpec.containsKey(partitionCol)) {
-				staticPartitionSpec.put(partitionCol, partitionSpec.get(partitionCol));
+				staticPartitionSpec.put(partitionCol, PlanUtils.stripQuotes(partitionSpec.get(partitionCol)));


Just curious, partitionSpec is a map of strings, and the value should probably have been unquoted before getting here. I'm wondering if the fix should be at a lower level than here. Otherwise, similar fix will be needed wherever such partition spec is used.

lirui-apache · 2019-08-23T06:46:39Z

PR updated to get unquoted partition values in RichSqlInsert, so that we don't have to do it in HiveTableSink.
@danny0405 better if you can also take a look. Thanks.

danny0405 · 2019-08-23T09:14:43Z

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dml/RichSqlInsert.java

@@ -75,7 +77,8 @@ public SqlNodeList getStaticPartitions() {
 		}
 		for (SqlNode node : this.staticPartitions.getList()) {
 			SqlProperty sqlProperty = (SqlProperty) node;
-			String value = SqlLiteral.value(sqlProperty.getValue()).toString();
+			Comparable comparable = SqlLiteral.value(sqlProperty.getValue());
+			String value = comparable instanceof NlsString ? ((NlsString) comparable).getValue() : comparable.toString();


I'm ok with the RichSqlInsert change.

xuefuz · 2019-08-23T16:20:22Z

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

+			hiveShell.insertInto("db1", "src").addRow(1).addRow(2).commit();
+			hiveShell.execute("create table db1.dest (x int) partitioned by (p1 string, p2 double)");
+			TableEnvironment tableEnv = getTableEnvWithHiveCatalog();
+			tableEnv.sqlUpdate("insert into db1.dest partition (p1='1''1', p2=1.1) select x from db1.src");


I'm wondering what this means and why a user might do this: p1='1''1'

Agree, a little confusing.

I agree this may not be common use case, but theoretically a string partition column can take any valid values including those with single quotes. And Hive also supports this.

lirui-apache · 2019-08-29T11:52:00Z

@xuefuz Do you have any further comments?

xuefuz · 2019-08-29T17:43:08Z

No. It looks good to me. Thanks.

bowenli86 · 2019-08-29T22:59:52Z

LGTM, merging

bowenli86 · 2019-08-29T23:40:38Z

The following test is failing on my machine:

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   TableEnvHiveConnectorTest.testStaticPartition:203->verifyHiveQueryResult:253 expected:<[2	1'1	1.1, 1	1'1	1.1]> but was:<[2	'1''1'	1.1, 1	'1''1'	1.1]>
[INFO]
[ERROR] Tests run: 235, Failures: 1, Errors: 0, Skipped: 0

…values

lirui-apache · 2019-08-30T06:39:52Z

@bowenli86 I can't reproduce the error you mentioned. Rebase to trigger travis again.

lirui-apache · 2019-08-30T12:19:39Z

Seems the latest Travis build passed.
@bowenli86 Could you please retry the failed test on your side, and make sure the sql-parser is built with this PR?

rmetzger added the review=description? label Aug 22, 2019

rmetzger added the component=Connectors/Hive label Aug 22, 2019

xuefuz reviewed Aug 22, 2019

View reviewed changes

lirui-apache force-pushed the FLINK-13814 branch from 2227892 to 5ad14b3 Compare August 23, 2019 06:29

danny0405 reviewed Aug 23, 2019

View reviewed changes

xuefuz reviewed Aug 23, 2019

View reviewed changes

lirui-apache force-pushed the FLINK-13814 branch from 5ad14b3 to 2215b05 Compare August 29, 2019 07:20

zjuwangg approved these changes Aug 29, 2019

View reviewed changes

xuefuz approved these changes Aug 29, 2019

View reviewed changes

lirui-apache added 2 commits August 30, 2019 14:03

[FLINK-13814][hive] HiveTableSink should strip quotes from partition …

3e4b5d3

…values

return unquoted values in RichSqlInsert

6901271

lirui-apache force-pushed the FLINK-13814 branch from 2215b05 to 6901271 Compare August 30, 2019 06:39

asfgit closed this in 07dfffe Aug 30, 2019

lirui-apache deleted the FLINK-13814 branch September 2, 2019 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-13814][hive] HiveTableSink should strip quotes from partition … #9502

[FLINK-13814][hive] HiveTableSink should strip quotes from partition … #9502

lirui-apache commented Aug 22, 2019

flinkbot commented Aug 22, 2019 •

edited

flinkbot commented Aug 22, 2019 •

edited

lirui-apache commented Aug 22, 2019

xuefuz Aug 22, 2019

lirui-apache commented Aug 23, 2019

danny0405 Aug 23, 2019

xuefuz Aug 23, 2019

danny0405 Aug 26, 2019

lirui-apache Aug 28, 2019

lirui-apache commented Aug 29, 2019

xuefuz commented Aug 29, 2019

bowenli86 commented Aug 29, 2019

bowenli86 commented Aug 29, 2019

lirui-apache commented Aug 30, 2019

lirui-apache commented Aug 30, 2019

[FLINK-13814][hive] HiveTableSink should strip quotes from partition … #9502

[FLINK-13814][hive] HiveTableSink should strip quotes from partition … #9502

Conversation

lirui-apache commented Aug 22, 2019

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 Aug 22, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Aug 22, 2019 • edited

CI report:

lirui-apache commented Aug 22, 2019

xuefuz Aug 22, 2019

Choose a reason for hiding this comment

lirui-apache commented Aug 23, 2019

danny0405 Aug 23, 2019

Choose a reason for hiding this comment

xuefuz Aug 23, 2019

Choose a reason for hiding this comment

danny0405 Aug 26, 2019

Choose a reason for hiding this comment

lirui-apache Aug 28, 2019

Choose a reason for hiding this comment

lirui-apache commented Aug 29, 2019

xuefuz commented Aug 29, 2019

bowenli86 commented Aug 29, 2019

bowenli86 commented Aug 29, 2019

lirui-apache commented Aug 30, 2019

lirui-apache commented Aug 30, 2019

flinkbot commented Aug 22, 2019 •

edited

flinkbot commented Aug 22, 2019 •

edited