Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug] [connector-jdbc] Nullable Column source have null data could be unexpected results. #5560

Merged
merged 1 commit into from
Oct 27, 2023
Merged

[Bug] [connector-jdbc] Nullable Column source have null data could be unexpected results. #5560

merged 1 commit into from
Oct 27, 2023

Conversation

mosence
Copy link
Contributor

@mosence mosence commented Sep 26, 2023 •
edited by Hisoka-X
Loading

Purpose of this pull request

Fix. #5559

Does this PR introduce any user-facing change?

yes

How was this patch tested?

Test case see the Issue #5559

Result:
image

Check list

liugddx
liugddx reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@liugddx liugddx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some test case

@mosence
Copy link
Contributor Author

mosence commented Sep 26, 2023

Please add some test case

I don't know how to code this test case. Only simulation and results of bugs were conducted.

@liugddx
Copy link
Member

liugddx commented Sep 26, 2023

You need merge dev and run CI test

@liugddx
Copy link
Member

liugddx commented Sep 26, 2023

You need merge dev and run CI test

image

@liugddx
Copy link
Member

liugddx commented Sep 26, 2023

Please add some test case

I don't know how to code this test case. Only simulation and results of bugs were conducted.

You can test that insert null and ensure that no errors will be reported when executing the task.

@mosence
Copy link
Contributor Author

mosence commented Sep 26, 2023

Please add some test case

I don't know how to code this test case. Only simulation and results of bugs were conducted.

You can test that insert null and ensure that no errors will be reported when executing the task.

The problem is that this bug is a data inconsistency issue, and null values do not cause errors.

@mosence
Copy link
Contributor Author

mosence commented Sep 26, 2023

Ci.

@mosence mosence force-pushed the fix-5559 branch 2 times, most recently from 5985c83 to c297c42 Compare September 27, 2023 02:26
@mosence mosence requested a review from liugddx September 27, 2023 04:06
liugddx
liugddx previously approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@liugddx liugddx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @ic4y PTAL

@Hisoka-X
Copy link
Member

Hisoka-X commented Oct 8, 2023

LGTM, but could you add some test case to avoid regression? Thanks!

@mosence
Copy link
Contributor Author

mosence commented Oct 9, 2023

LGTM, but could you add some test case to avoid regression? Thanks!

I have not found any existing test cases for data consistency, and currently I do not have time to rewrite one. If there are similar test cases in the future, I can supplement them.

@Hisoka-X
Copy link
Member

LGTM, but could you add some test case to avoid regression? Thanks!

I have not found any existing test cases for data consistency, and currently I do not have time to rewrite one. If there are similar test cases in the future, I can supplement them.

OK for me. Could you add a issue for this TODO?

EricJoy2048
EricJoy2048 previously approved these changes Oct 11, 2023
View reviewed changes
@Hisoka-X
Copy link
Member

After a second consider, I think you can modify exist e2e test case to cover this PR. For example, use postgres, in https://github.com/apache/seatunnel/tree/dev/seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc, you can add one row with null value data, then check it in sink postgres table after e2e job fininshed.

Hisoka-X
Hisoka-X requested changes Oct 12, 2023
View reviewed changes
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test case

@mosence
Copy link
Contributor Author

mosence commented Oct 13, 2023

please add test case

There is no existing test case that implements abstract void compareResult() throws SQLException, IOException; Interface.

I am really not familiar with this testing framework.

@liugddx
Copy link
Member

liugddx commented Oct 13, 2023

LGTM, but could you add some test case to avoid regression? Thanks!

I have not found any existing test cases for data consistency, and currently I do not have time to rewrite one. If there are similar test cases in the future, I can supplement them.

OK for me. Could you add a issue for this TODO?

You need to create an issue, which can be used as another task

@mosence
Copy link
Contributor Author

mosence commented Oct 13, 2023 •
edited
Loading

I submit an issue :P
@liugddx

#5623

@Hisoka-X
Copy link
Member

I think the code of this PR should be fine and run normally, but we really need automated test cases to avoid regressions. Maybe you can refer to the test code that has been implemented in #4812 . cc @choucmei

@mosence mosence dismissed stale reviews from EricJoy2048 and liugddx via 0429980 October 16, 2023 07:51
@mosence mosence force-pushed the fix-5559 branch 2 times, most recently from 0429980 to 7e0de9f Compare October 16, 2023 08:35
@Hisoka-X
Copy link
Member

Could you push the test case code? So that we can find the problem with you.

@mosence
Copy link
Contributor Author

mosence commented Oct 26, 2023 •
edited
Loading

Could you push the test case code? So that we can find the problem with you.

Commit e1ec769.

@mosence
Copy link
Contributor Author

mosence commented Oct 26, 2023

@Hisoka-X
And my commit auto test.
I found that postgreSQL is extremely prone to data inconsistency issues:

Test org.apache.seatunnel.connectors.seatunnel.jdbc.JdbcPostgresIT.testAutoGenerateSQL failed with:
org.opentest4j.AssertionFailedError: iterable lengths differ, expected: <1000> but was: <659>

@Hisoka-X
Copy link
Member

I found that postgreSQL is extremely prone to data inconsistency issues:

Already fixed on dev, could you try merge from dev?

@mosence
Copy link
Contributor Author

mosence commented Oct 26, 2023

I found that postgreSQL is extremely prone to data inconsistency issues:

Already fixed on dev, could you try merge from dev?

This is run on yesterday. Now I update my branch.

mosence
mosence commented Oct 26, 2023
View reviewed changes
Copy link
Contributor Author

@mosence mosence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests, but did not match expectations, resulting in other issues and inconsistent data

@mosence mosence mentioned this pull request Oct 27, 2023
Merged
4 tasks
@mosence
Copy link
Contributor Author

mosence commented Oct 27, 2023 •
edited
Loading

Could you push the test case code? So that we can find the problem with you.
@Hisoka-X
I add test case code in PR #5734.

@mosence mosence requested a review from Hisoka-X October 27, 2023 06:37
@Hisoka-X
Copy link
Member

Waiting CI pass

@Hisoka-X
Copy link
Member

BTW, we should make sure #5734 be merged ASAP.

@mosence
Copy link
Contributor Author

mosence commented Oct 27, 2023 •
edited
Loading

BTW, we should make sure #5734 be merged ASAP.

Yeah, I agree for you.
The data consistency of integrated tools is quite important.

@hailin0 hailin0 merged commit 3f429e1 into apache:dev Oct 27, 2023
@mosence mosence deleted the fix-5559 branch October 30, 2023 01:31
@Hisoka-X Hisoka-X mentioned this pull request Oct 30, 2023
Closed
3 tasks
@tedshim tedshim mentioned this pull request Nov 27, 2023
Closed
4chicat pushed a commit to nexr/incubator-seatunnel that referenced this pull request Nov 29, 2023
@mosence @4chicat
[Bug] [connector-jdbc] Nullable Column source have null data could be…
f984489 
… unexpected results. (apache#5560)
tedshim pushed a commit to nexr/incubator-seatunnel that referenced this pull request Dec 1, 2023
@mosence @tedshim
[Bug] [connector-jdbc] Nullable Column source have null data could be…
5db508a 
… unexpected results. (apache#5560)
@tedshim tedshim mentioned this pull request Dec 1, 2023
Merged
4chicat pushed a commit to nexr/incubator-seatunnel that referenced this pull request Dec 1, 2023
@tedshim @mosence
[Bug] [connector-jdbc] Nullable Column source have null data could be…
3be869e 
… unexpected results. (apache#5560) (#5)

Co-authored-by: MoSence <[email protected]>
@Hisoka-X Hisoka-X mentioned this pull request Mar 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hisoka-X Hisoka-X Hisoka-X approved these changes

@hailin0 hailin0 hailin0 approved these changes

@choucmei choucmei Awaiting requested review from choucmei

@EricJoy2048 EricJoy2048 Awaiting requested review from EricJoy2048

@liugddx liugddx Awaiting requested review from liugddx

Assignees
No one assigned
Labels
approved need add test case reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mosence @liugddx @Hisoka-X @ic4y @choucmei @hailin0 @EricJoy2048