-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
[Fix][StarRocks] Fix NPE when upstream catalogtable table path only have table name part #6540
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test case for this?
@Hisoka-X in the current code, there has the NPE check https://github.com/apache/seatunnel/pull/6540/files#diff-ab5c2b43dd112473304501aa777993ab9cd746f3a12b6606f87523d92b5e9086R117. In this method i think the author is missing to check. |
Since this is a bug, then we could have the means to reproduce it. It is best to use test cases to reproduce the bug, so as to ensure that there will be no regression in the future. |
Thanks @liunaijie for update test case! I suggest use fake source connector to produce table with only table name. There are example https://github.com/apache/seatunnel/blob/cdfb5735acfbd281e01bf34393870fa573c5516c/docs/en/concept/schema-feature.md#table. It will be more simple and stable. |
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-starrocks-e2e/pom.xml
Outdated
Show resolved
Hide resolved
if (StringUtils.isNotBlank(sourceDatabaseName)) { | ||
sinkDatabaseName = | ||
sinkDatabaseName.replace(REPLACE_DATABASE_NAME_KEY, sourceDatabaseName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer when source database name is null, we can treat it as empty string.
if (StringUtils.isNotBlank(sourceDatabaseName)) { | |
sinkDatabaseName = | |
sinkDatabaseName.replace(REPLACE_DATABASE_NAME_KEY, sourceDatabaseName); | |
} | |
sinkDatabaseName = sinkDatabaseName.replace(REPLACE_DATABASE_NAME_KEY, sourceDatabaseName != null ? sourceDatabaseName: ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Please fix ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if ci passed.
I checked other connectors, they all have null value check. |
Yes, could you try to reopen it? |
…ave table name part (apache#6540)
Purpose of this pull request
close #6524
in some source connector, it not set db name, so will get NPE here
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.