[HUDI-485]: corrected the check for incremental sql by pratyakshsharma · Pull Request #2768 · apache/hudi

pratyakshsharma · 2021-04-04T17:43:27Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

Corrected the check for incremental SQL in HiveIncrementalPuller.java class.

Brief change log

(for example:)

Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

codecov-io · 2021-04-04T18:29:30Z

Codecov Report

Merging #2768 (5d32a15) into master (e970e1f) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2768      +/-   ##
============================================
- Coverage     52.32%   52.30%   -0.02%     
  Complexity     3689     3689              
============================================
  Files           483      483              
  Lines         23095    23095              
  Branches       2460     2460              
============================================
- Hits          12084    12080       -4     
- Misses         9942     9946       +4     
  Partials       1069     1069

Flag	Coverage Δ	Complexity Δ
hudicli	`37.01% <ø> (ø)`	`0.00 <ø> (ø)`
hudiclient	`∅ <ø> (∅)`	`0.00 <ø> (ø)`
hudicommon	`50.78% <ø> (-0.05%)`	`0.00 <ø> (ø)`
hudiflink	`56.71% <ø> (ø)`	`0.00 <ø> (ø)`
hudihadoopmr	`33.44% <ø> (ø)`	`0.00 <ø> (ø)`
hudisparkdatasource	`71.33% <ø> (ø)`	`0.00 <ø> (ø)`
hudisync	`45.47% <ø> (ø)`	`0.00 <ø> (ø)`
huditimelineservice	`64.36% <ø> (ø)`	`0.00 <ø> (ø)`
hudiutilities	`69.74% <ø> (+0.05%)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ache/hudi/common/fs/inline/InMemoryFileSystem.java	`79.31% <0.00%> (-10.35%)`	`15.00% <0.00%> (-1.00%)`
...e/hudi/common/table/log/HoodieLogFormatWriter.java	`78.12% <0.00%> (-1.57%)`	`26.00% <0.00%> (ø%)`
...apache/hudi/utilities/deltastreamer/DeltaSync.java	`71.42% <0.00%> (+0.34%)`	`56.00% <0.00%> (+1.00%)`

vinothchandar · 2021-04-19T22:33:12Z

@vingov for an extra pair of eyes. @pratyakshsharma any testing done for the PR? any way to add a test for the tool?

vingov · 2021-04-20T05:11:42Z

lgtm, we should use '%s' to replace the from commit time in the line no. 197 with the string format function:

incrementalPullSQLtemplate.add("incrementalSQL", String.format(incrementalSQL, config.fromCommitTime));

nsivabalan · 2021-05-21T15:57:03Z

@pratyakshsharma : can you please respond to vinoth's feedback when you get a chance.

pratyakshsharma · 2021-05-29T19:17:29Z

@vingov for an extra pair of eyes. @pratyakshsharma any testing done for the PR? any way to add a test for the tool?

No I have not done any testing. Currently there is no test coverage for this tool. The lines of code that this PR is touching do not need any test case I believe. We can try to cover the entire tool in general with unit tests though. Do you want me to do that as part of this PR itself? @vinothchandar

nsivabalan · 2021-06-05T11:14:23Z

yes, please.

vinothchandar · 2021-09-07T21:39:29Z

I see L197 is not yet fixed. @vingov given you use this or a similar tool daily. Do you want to upstream a patch for this?

codope

@pratyakshsharma This should be good to land. Could you please resolve the conflict and address the comment by @vingov ?

vingov · 2021-09-09T00:25:14Z

the code looks good, my last comment was confirming that we should use '%s' so that the String.format function in the L197 will replace it correctly.

No further changes are required by @pratyakshsharma!

pratyakshsharma · 2021-09-12T16:03:53Z

@codope I need to add few test cases for this PR. Will do that soon!

nsivabalan · 2021-12-13T14:09:55Z

@pratyakshsharma : Can you please follow up on this and address the feedback.

vinothchandar · 2021-12-21T00:39:39Z

@pratyakshsharma can we please see this PR through? are you still interested in driving this forward?

pratyakshsharma · 2021-12-21T05:21:01Z

Ack. Let me close this in a day or two.

pratyakshsharma · 2021-12-21T14:59:01Z

@nsivabalan @vinothchandar please take a pass. We can include this in 0.10.1 release as well.

vinothchandar

Few nits. but okay with it overall. @codope Can take it from here.

vinothchandar · 2021-12-22T03:28:04Z

+  }
+
+  @Test
+  public void testPullerWithoutIncrementalClause() throws IOException, URISyntaxException {


these tests only seem to be testing some failure scenarios? not the happy path?

I was expecting this comment :)

I am actually facing some error with the happy flow test case. Once I am able to fix it, will add it.

@pratyakshsharma Is the patch ready? If not, can you please update the happy flow test case even if it's failing. I can take over and try to fix it.

pratyakshsharma · 2022-01-11T18:15:23Z

@codope please take a pass, I have added the happy flow test case now.

hudi-bot · 2022-01-11T19:22:36Z

CI report:

f115b91 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

codope

Thanks for addressing the comments and adding the test. Looks good.

* [HUDI-485]: corrected the check for incremental sql * [HUDI-485]: added tests * code review comments addressed * [HUDI-485]: added happy flow test case

[HUDI-485]: corrected the check for incremental sql

5d32a15

vinothchandar self-assigned this Apr 19, 2021

nsivabalan added the priority:high Significant impact; potential bugs label May 11, 2021

vinothchandar assigned nsivabalan and unassigned vinothchandar Sep 7, 2021

codope requested changes Sep 8, 2021

View reviewed changes

rebased with master

ffa7547

nsivabalan assigned codope and unassigned nsivabalan Dec 13, 2021

pratyakshsharma added 2 commits December 21, 2021 11:19

Merge branch 'master' of https://github.com/apache/hudi into hudi-485

f7b07f9

[HUDI-485]: added tests

1b383db

vinothchandar approved these changes Dec 22, 2021

View reviewed changes

pratyakshsharma added 2 commits December 23, 2021 00:58

code review comments addressed

45ca9a5

[HUDI-485]: added happy flow test case

f115b91

codope approved these changes Jan 12, 2022

View reviewed changes

codope merged commit a392e9b into apache:master Jan 12, 2022

vinishjail97 mentioned this pull request Jan 24, 2022

FixIgnoreKey nsivabalan/hudi#11

Closed

5 tasks

Conversation

pratyakshsharma commented Apr 4, 2021

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

codecov-io commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vinothchandar commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vingov commented Apr 20, 2021

Uh oh!

nsivabalan commented May 21, 2021

Uh oh!

pratyakshsharma commented May 29, 2021

Uh oh!

nsivabalan commented Jun 5, 2021

Uh oh!

vinothchandar commented Sep 7, 2021

Uh oh!

codope 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!

vingov commented Sep 9, 2021

Uh oh!

pratyakshsharma commented Sep 12, 2021

Uh oh!

nsivabalan commented Dec 13, 2021

Uh oh!

vinothchandar commented Dec 21, 2021

Uh oh!

pratyakshsharma commented Dec 21, 2021

Uh oh!

pratyakshsharma commented Dec 21, 2021

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vinothchandar Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

codope Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pratyakshsharma commented Jan 11, 2022

Uh oh!

hudi-bot commented Jan 11, 2022

CI report:

Uh oh!

codope left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Apr 4, 2021 •

edited

Loading

vinothchandar commented Apr 19, 2021 •

edited

Loading

codope left a comment •

edited

Loading