[improve] update localfile connector config#8765
Conversation
# Conflicts: # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/BaseFileSinkConfig.java # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSinkOptions.java # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategy.java # seatunnel-connectors-v2/connector-file/connector-file-cos/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/cos/sink/CosFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-cos/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/cos/source/CosFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-ftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/ftp/sink/FtpFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-ftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/ftp/source/FtpFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-hadoop/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/hdfs/sink/HdfsFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-hadoop/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/hdfs/source/HdfsFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-jindo-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/jindo/sink/OssFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-jindo-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/jindo/source/OssFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-local/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/local/sink/LocalFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-local/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/local/source/LocalFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-obs/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/obs/sink/ObsFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-obs/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/obs/source/ObsFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/source/OssFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-s3/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/s3/source/S3FileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-sftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/sink/SftpFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-sftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/source/SftpFileSourceFactory.java
liunaijie
left a comment
There was a problem hiding this comment.
LGTM, thanks @misi1987107 !
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the localfile connector configuration by renaming and refactoring configuration option classes from Base* to FileBase* and synchronizing key references across source, sink, and test files. Key changes include:
- Renaming configuration classes and updating option key references.
- Adjustments to option descriptions and default values in connector source and sink implementations.
- Updates to test cases and read/write strategies to use the new FileBase* option classes.
Reviewed Changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSourceOptions.java | Added several new option definitions and updated descriptions. |
| connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseOptions.java | Renamed from BaseSourceConfigOptions and updated option descriptions. |
| connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSinkOptions.java | Renamed and updated option key references in sink config. |
| Various source/reader/sink files | Updated to use FileBase* options instead of Base* options. |
| ConnectorOptionCheckTest.java | Added additional type checks to support the new option classes. |
Comments suppressed due to low confidence (2)
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseOptions.java:40
- The description for the file path option was changed from 'source files' to 'target files', which may be inconsistent with the connector's intended usage.
.withDescription("The file path of target files");
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/CsvReadStrategy.java:190
- The hard-coded assignment of fieldDelimiter to ',' overwrites any configured value from the plugin configuration, which could lead to unexpected behavior.
fieldDelimiter = ",";
| .optional(BaseSinkConfig.SCHEMA_SAVE_MODE) | ||
| .optional(BaseSinkConfig.DATA_SAVE_MODE) | ||
| .optional(FileBaseSinkOptions.TMP_PATH) | ||
| .optional(FileBaseSinkOptions.FILE_FORMAT_TYPE) | ||
| .optional(FileBaseSinkOptions.SCHEMA_SAVE_MODE) | ||
| .optional(FileBaseSinkOptions.DATA_SAVE_MODE) | ||
| .conditional( | ||
| BaseSinkConfig.FILE_FORMAT_TYPE, | ||
| FileFormat.TEXT, | ||
| BaseSinkConfig.ROW_DELIMITER, | ||
| BaseSinkConfig.FIELD_DELIMITER, | ||
| BaseSinkConfig.TXT_COMPRESS, | ||
| BaseSinkConfig.ENABLE_HEADER_WRITE) | ||
| .conditional( | ||
| BaseSinkConfig.FILE_FORMAT_TYPE, | ||
| FileFormat.CSV, | ||
| BaseSinkConfig.TXT_COMPRESS, | ||
| BaseSinkConfig.ENABLE_HEADER_WRITE) | ||
| .conditional( | ||
| BaseSinkConfig.FILE_FORMAT_TYPE, | ||
| FileFormat.JSON, | ||
| BaseSinkConfig.TXT_COMPRESS) | ||
| .conditional( | ||
| BaseSinkConfig.FILE_FORMAT_TYPE, | ||
| FileFormat.ORC, | ||
| BaseSinkConfig.ORC_COMPRESS) | ||
| .conditional( | ||
| BaseSinkConfig.FILE_FORMAT_TYPE, | ||
| FileFormat.PARQUET, | ||
| BaseSinkConfig.PARQUET_COMPRESS, | ||
| BaseSinkConfig.PARQUET_AVRO_WRITE_FIXED_AS_INT96, | ||
| BaseSinkConfig.PARQUET_AVRO_WRITE_TIMESTAMP_AS_INT96) |
There was a problem hiding this comment.
I found so many conditional be deleted. Why should do that? @misi1987107 . cc @davidzollo @liunaijie
There was a problem hiding this comment.
I think the
conditional( @nonnull Option conditionalOption,
@nonnull T expectValue,
@nonnull Option<?>... requiredOptions)
method is a required parameter that meets the specified conditions, but these are not required options,
So I deleted these.
Such as FILE_FORMAT_TYPE is Text,but TXT_COMPRESS is not a required parameter.
Did I misunderstand,give me some pointer?
Purpose of this pull request
subtask of #8576
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note.