Skip to content

Conversation

@corgy-w
Copy link
Contributor

@corgy-w corgy-w commented Apr 18, 2025

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@corgy-w corgy-w marked this pull request as ready for review April 18, 2025 09:42
@nielifeng nielifeng requested a review from Copilot April 21, 2025 02:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes an issue with the Kafka connector’s database naming by refactoring the table path construction logic. Key changes include:

  • Extraction of the table path resolution into a new method (getTablePathFromSchema) for improved flexibility.
  • Updates to the catalog table creation to use the new table path resolution.
  • Addition of a new unit test (testDeserializationWithSchema) to validate deserialization behavior when a schema is provided.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
seatunnel-connectors-v2/connector-kafka/src/test/java/org/apache/seatunnel/connectors/seatunnel/kafka/source/KafkaSourceConfigTest.java Added a unit test to verify deserialization with a provided schema and table identifier.
seatunnel-connectors-v2/connector-kafka/src/main/java/org/apache/seatunnel/connectors/seatunnel/kafka/source/KafkaSourceConfig.java Refactored the table path construction by extracting logic into getTablePathFromSchema to fix database naming issues.
Comments suppressed due to low confidence (2)

seatunnel-connectors-v2/connector-kafka/src/main/java/org/apache/seatunnel/connectors/seatunnel/kafka/source/KafkaSourceConfig.java:265

  • Ensure that the schema configuration key is used consistently across the connector. You might consider aligning the usage of KafkaSourceOptions.SCHEMA and TableSchemaOptions.SCHEMA to prevent potential misconfiguration.
readonlyConfig.getOptional(TableSchemaOptions.SCHEMA)

seatunnel-connectors-v2/connector-kafka/src/test/java/org/apache/seatunnel/connectors/seatunnel/kafka/source/KafkaSourceConfigTest.java:86

  • [nitpick] Consider clarifying the test table identifier format (e.g., separating database and table parts) to ensure it aligns with production expectations and avoids ambiguity.
schema.put(TABLE.key(), "test.test");

Copy link
Member

@hailin0 hailin0 left a comment

Choose a reason for hiding this comment

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

LGTM

waiting for ci passed

@Hisoka-X Hisoka-X merged commit 79d9a93 into apache:dev Apr 22, 2025
5 checks passed
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.

3 participants