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.

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

[Improve][connector-file] unifiy option between file source/sink and update document #5680

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

liunaijie
Copy link
Member

@liunaijie liunaijie commented Oct 20, 2023

Purpose of this pull request

close #5461 and #4572

  1. unified option in file source/sink (source parameter delimiter update to field_delimiter)
  2. update the doucment
    a) update parameter name
    b) refactor docs using unified format

Does this PR introduce any user-facing change?

yes, the file source paramter delimiter updated to field_delimiter to keep same with sink paramter

How was this patch tested?

use e2e

Check list

- unified field_delimiter parameter
- document update
@Hisoka-X
Copy link
Member

So after this PR, seem like delimiter will not worked. This is unacceptable and will hinder users who need to upgrade. If we need to modify a configuration key, we should be compatible with the old configuration at the same time.

@Hisoka-X
Copy link
Member

Hisoka-X commented Oct 21, 2023

So I think you should answer this question.
image

Also please update the title.

@liunaijie liunaijie changed the title File option [Improve][connector-file] unifiy option between file source/sink and update document Oct 21, 2023
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.

Could you add a test case for old config key delimiter? We should make sure the delimiter work fine too after this change.

Comment on lines 73 to 103
sink {
Assert {
rules {
row_rules = [
{
rule_type = MAX_ROW
rule_value = 5
}
],
field_rules = [
{
field_name = c_string
field_type = string
field_value = [
{
rule_type = NOT_NULL
}
]
},
{
field_name = c_boolean
field_type = boolean
field_value = [
{
rule_type = NOT_NULL
}
]
}
]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The test case seem like can not prove the delimiter is ":". Could you add a rule to check row value? You can refer https://github.com/apache/seatunnel/pull/5083/files#diff-a68b2797462655fd5ee139c5c129df223f4de9c25e72e41a6432012d00ede0a8

@Hisoka-X
Copy link
Member

Thanks @liunaijie .Waiting CI pass.

@EricJoy2048 EricJoy2048 merged commit 8d87cf8 into apache:dev Oct 30, 2023
6 checks passed
@liunaijie liunaijie deleted the file_option branch October 31, 2023 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][FileConnector] Unify some option in file source/sink connector
3 participants