chovy-3012 commented on PR #12514: URL: https://github.com/apache/seatunnel/pull/12514#issuecomment-6109286638
Thanks for the review @davidzollo! Here is where each of the three issues stands: ### Issue 1 — CRLF line endings (Blocking) This was reviewed on the old head `b4b6b082`, which had CRLF. That commit has since been overwritten — the current head is `caf523671`. Both files are now LF-only (`git diff --check origin/dev...HEAD` passes clean, diff is table-only — 84 changed lines, no whole-file rewrite). This should have already been resolved between your review and that commit being replaced. ### Issue 2 — 7 options not declared in `CosFileSourceFactory.optionRule()` (Blocking) I traced all seven options through the `connector-file-base` module. They are not just Option definitions — they are actively read and consumed by code that `CosFileSource` inherits: | Option | Read at | Used for | |--------|---------|----------| | `read_partitions` | `AbstractReadStrategy.java:349` | Filters files in `isSourceCandidate` — only keeps files whose path matches a configured partition | | `csv_use_header_line` | `CsvReadStrategy.java:296` | Controls whether the CSV parser treats the first row as a header (`withFirstRecordAsHeader()`) | | `binary_chunk_size` | `BinaryReadStrategy.java:74` | Sets the read buffer size for binary files (`new byte[binaryChunkSize]`), validates >0 and ≤100 MB | | `binary_complete_file_mode` | `BinaryReadStrategy.java:88` | Branches between reading the entire file as one chunk vs. chunked reading | | `file_filter_modified_start` | `AbstractReadStrategy.java:369` | Filters out files whose modification timestamp is before the configured start time | | `file_filter_modified_end` | `AbstractReadStrategy.java:375` | Filters out files whose modification timestamp is at or after the configured end time | | `sort_files_by_modification_time` | `AbstractReadStrategy.java:385` | Sorts discovered files by modification time descending so schema inference picks the latest file | The consumption path is: `CosFileSource extends BaseFileSource`, whose constructor (line 77) calls `readStrategy.setPluginConfig(pluginConfig.toConfig())`, passing the full user config to the strategy. `AbstractReadStrategy.setPluginConfig` then reads options 1, 5, 6, 7 via `pluginConfig.hasPath()` / `.getString()` / `.getBoolean()` / `.getInt()`. Options 2, 3, 4 are read conditionally by `CsvReadStrategy` and `BinaryReadStrategy` respectively (matching their documented scope: "only used when file_format is csv/binary"). This gap (Option defined but not declared in the factory rule) exists across **all** file source factories (Local, S3, Oss, Hdfs, Ftp), not just COS. The options work in normal Zeta runs — only the `-v` / Flink/Spark validation path rejects them via `validateUnknownKeys`. I have a fix ready locally: adding the 7 `.optional()` declarations to `CosFileSourceFactory.optionRule()`. It is backward-compatible (no runtime behavior change — the code already reads these options). Would you like me to include it in this PR, or would you prefer a separate PR that fixes all file connectors? ### Issue 3 — Blank Description column for core required fields (Non-blocking) Fixed in commit `caf523671`. Added descriptions for all 8 previously-blank fields in both en and zh files: `path`, `bucket`, `secret_id`, `secret_key`, `region`, `date_format`, `datetime_format`, `time_format`. No blank Description cells remain in the options table. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
