[FLINK-13012][hive] Handle default partition name of Hive table #9088

lirui-apache · 2019-07-11T13:14:50Z

What is the purpose of the change

To handle default partition name when reading/writing Hive tables.

Brief change log

Handle default partition name.
Added a new test case for default partition name.

Verifying this change

Comes with new 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): yes
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

lirui-apache · 2019-07-11T13:16:52Z

The newly added test is disabled for now. I'll update this PR once #9039 is in.

flinkbot · 2019-07-11T13:17:14Z

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 cbc2d2b (Wed Aug 07 08:16:29 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.

Details

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-07-11T13:26:05Z

CI report:

eac5f74 : FAILURE Build
b22f836 : SUCCESS Build
64007bc : FAILURE Build
efd6e31 : FAILURE Build
d8b3088 : FAILURE Build
cbc2d2b : SUCCESS Build

lirui-apache · 2019-07-16T08:33:15Z

cc @xuefuz @bowenli86 @zjuwangg
This PR handles default partition name as well as adding e2e test case. Let me know if you think we should split it into separate PRs.
And I changed hive-exec dependency to compile-scope in hive connector. It's because provided deps are not transitive, and therefore anything depending on hive connector needs to declare its own dependency on hive-exec and keep in sync with hive connector. I suppose we don't need to worry about licenses as long as we don't bundle hive in our jars, is that correct?

bowenli86

@lirui-apache thanks for your contribution!

It would be good to split it into two PRs, one for partition name, one for e2e tests with two commits with their own JIRAs as 1st change dependencies of flink-connector-hive/pom.xml and 2nd add e2e test module. We may need to ask Chesnay to help review how to setup e2e on Travis.

IIUIC, changing from 'provided' to 'compile' should be fine.

BTW, do we have plan to add e2e tests for Hive 1.2.1?

flink-connectors/flink-connector-hive/pom.xml

...nnector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableOutputFormat.java

...ink-connector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableSource.java

bowenli86 · 2019-07-16T16:39:47Z

...ink-connector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableSource.java

+						if (defaultPartitionName.equals(partitionValue)) {
+							LogicalTypeRoot typeRoot = type.getLogicalType().getTypeRoot();
+							// while this is inline with Hive, seems it should be null for string columns as well?
+							partitionObject = typeRoot == LogicalTypeRoot.CHAR || typeRoot == LogicalTypeRoot.VARCHAR ? defaultPartitionName : null;


Wondering why partitionObject will be null if the type root is not char/varchar?

If the partition value equals default partition name, it means the partition value should actually be null.

flink-end-to-end-tests/flink-connector-hive-test/pom.xml

xuefuz · 2019-07-16T16:47:34Z

...ink-connector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableSource.java

+						Object partitionObject;
+						if (defaultPartitionName.equals(partitionValue)) {
+							LogicalTypeRoot typeRoot = type.getLogicalType().getTypeRoot();
+							// while this is inline with Hive, seems it should be null for string columns as well?


Hive also has String type. What's Hive's behavior on this?

For a string partition column, it'll be assigned the default value, e.g. __HIVE_DEFAULT_PARTITION__. But I thought it should simply be null, just like other kinds of partition columns.

...nnector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableOutputFormat.java

xuefuz · 2019-07-16T16:59:09Z

Re: dep scope change

I guess it's a bigger change at this point. If I understand correctly, you are trying to make the pom file for the test project simpler. The motivation seems fine, but it invalidates the purpose of us putting "provided" there in the place, right? What's the consequence to the user with this change?

lirui-apache · 2019-07-22T04:17:00Z

Changed to test with table env. This PR needs to wait until we have #9181 merged.

lirui-apache · 2019-07-25T11:19:04Z

PR rebased.
@xuefuz @zjuwangg please take another look. Thanks.

xuefuz

LGTM.

For the Hive behavior, maybe we can create a JIRA, hoping to get what Hive folks say about it.

lirui-apache · 2019-07-26T02:06:04Z

Thanks @xuefuz.
@wuchong could you please help review and merge this PR. Travis test has passed: https://travis-ci.com/flink-ci/flink/builds/120691298

wuchong

Thanks @lirui-apache , it looks good to me.

Will merge this.

lirui-apache · 2019-07-29T07:15:10Z

Thanks @wuchong for help with the merge!

This closes #9088

This closes apache#9088

rmetzger added the review=description? label Jul 11, 2019

rmetzger added the component=Connectors/Hive label Jul 11, 2019

lirui-apache force-pushed the FLINK-13012 branch 2 times, most recently from b22f836 to 64007bc Compare July 16, 2019 08:05

bowenli86 reviewed Jul 16, 2019

View reviewed changes

xuefuz reviewed Jul 16, 2019

View reviewed changes

...nnector-hive/src/main/java/org/apache/flink/batch/connectors/hive/HiveTableOutputFormat.java Outdated Show resolved Hide resolved

lirui-apache mentioned this pull request Jul 19, 2019

[FLINK-13303][hive] Add e2e test for hive connector #9149

Closed

lirui-apache force-pushed the FLINK-13012 branch from efd6e31 to d8b3088 Compare July 22, 2019 04:13

lirui-apache added 7 commits July 25, 2019 16:48

[FLINK-13012][hive] Handle default partition name of Hive table

96743c7

disable dep convergence check

9fca5a9

enable test

a851944

revert unnecessary changes

c444a91

add test with table env

aab736b

address comments

3955f97

adapt the test

cbc2d2b

lirui-apache force-pushed the FLINK-13012 branch from d8b3088 to cbc2d2b Compare July 25, 2019 11:15

xuefuz approved these changes Jul 25, 2019

View reviewed changes

wuchong approved these changes Jul 29, 2019

View reviewed changes

asfgit closed this in 1d81374 Jul 29, 2019

lirui-apache deleted the FLINK-13012 branch July 29, 2019 07:17

asfgit pushed a commit that referenced this pull request Jul 29, 2019

[FLINK-13012][hive] Handle default partition name of Hive table

25e6143

This closes #9088

wzhero1 pushed a commit to wzhero1/flink that referenced this pull request Aug 2, 2019

[FLINK-13012][hive] Handle default partition name of Hive table

f2ebe73

This closes apache#9088

[FLINK-13012][hive] Handle default partition name of Hive table #9088

[FLINK-13012][hive] Handle default partition name of Hive table #9088

Uh oh!

Conversation

lirui-apache commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

lirui-apache commented Jul 11, 2019

Uh oh!

flinkbot commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

lirui-apache commented Jul 16, 2019

Uh oh!

bowenli86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bowenli86 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

lirui-apache Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xuefuz Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

lirui-apache Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xuefuz commented Jul 16, 2019

Uh oh!

lirui-apache commented Jul 22, 2019

Uh oh!

lirui-apache commented Jul 25, 2019

Uh oh!

xuefuz left a comment

Choose a reason for hiding this comment

Uh oh!

lirui-apache commented Jul 26, 2019

Uh oh!

wuchong left a comment

Choose a reason for hiding this comment

Uh oh!

lirui-apache commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lirui-apache commented Jul 11, 2019 •

edited

Loading

flinkbot commented Jul 11, 2019 •

edited

Loading

flinkbot commented Jul 11, 2019 •

edited

Loading

bowenli86 left a comment •

edited

Loading