Skip to content

Conversation

@LiJie20190102
Copy link
Contributor

Purpose of this pull request

Refer to the StarRocks/starrocks-connector-for-apache-flink#344 of using starrocks-connector-for-apache-flink

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@davidzollo
Copy link
Contributor

Changes Requested

Thanks for the contribution. The refactoring of joinRows looks good, but there are a few critical issues that need to be addressed before merging:

  1. Logic Inversion: The logic for wrap_json_as_array seems inverted.

    • Currently: if (!sinkConfig.isSinkWrapJsonAsArray()) triggers the wrapping logic. Since the default is false, !false is true, so it wraps by default.
    • Issue: If a user sets wrap_json_as_array = true, the code enters the else block (no wrapping). This is counter-intuitive.
    • Suggestion: Rename it to something like enable_json_array_wrapping (default true) or invert the if condition to match the current name semantics.
  2. Missing Tests: Please add unit tests in StarRocksStreamLoadVisitorTest to verify that the output format is correct when the option is enabled vs. disabled.

  3. PR Title: This change affects the Sink, not the Source. Please update the PR title to [Improve][Connector-V2][starrocks-sink]....

@LiJie20190102
Copy link
Contributor Author

After consideration and relevant testing, it was found that this issue does not occur in the sratunnel scenario, meaning that this type of data does not appear. I'll close this PR later

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.

2 participants