yigitcan-ozturk commented on issue #11007:
URL: https://github.com/apache/seatunnel/issues/11007#issuecomment-5903338239

   Thanks  I audited the current `dev` Source path and will keep this to one 
focused slice.
   
   Proposed first slice: ClickHouse Source `host` nonblank validation only.
   
   Current `dev` behavior
   ----------------------
   `ClickhouseSourceFactory.optionRule()` currently declares:
   
       .required(HOST, USERNAME, PASSWORD)
   
   so `host` is required for presence, but there is no declarative nonblank 
constraint.
   
   `ClickhouseSourceConfig.of(...)` then reads the value directly and 
`createSource(...)` passes it into `ClickhouseUtil.createNodes(...)`. I did not 
find an earlier Source-side validation that rejects an empty or whitespace-only 
`host` before connection/node construction.
   
   I am deliberately not including `batch_size` or `split.size` in this slice. 
`ClickhouseTableConfig.of(...)` currently treats values <= 0 as 
fallback-to-default:
   
       if (tableConfig.getBatchSize() <= 0) {
           tableConfig.setBatchSize(CLICKHOUSE_BATCH_SIZE.defaultValue());
       }
   
       if (tableConfig.getSplitSize() <= 0) {
           tableConfig.setSplitSize(CLICKHOUSE_SPLIT_SIZE.defaultValue());
       }
   
   Turning those into declarative numeric rejection would therefore change 
established behavior rather than just move an existing validation boundary.
   
   I am also not including `table_path` / `sql`: their current contract is 
relational (`table_path` and `sql` cannot both be empty), and SQL/table-path 
selection also affects the existing source strategy, so I would leave that out 
of this focused PR.
   
   Proposed declarative constraint
   -------------------------------
   Source only:
   
       required(HOST, Conditions.notBlank(HOST))
   
   No Sink or ClickhouseFile changes.
   
   Focused ConfigValidator/factory test boundary
   ---------------------------------------------
   For `host`:
   
   - valid nonblank host -> accepted
   - missing host -> rejected
   - empty host -> rejected
   - whitespace-only host -> rejected
   - padded nonblank host -> accepted
   
   Existing required-option behavior for `username` and `password` remains 
unchanged in this slice.
   
   Explicitly unchanged
   --------------------
   This PR would not change:
   
   - ClickHouse connection/node construction
   - authentication behavior
   - username/password semantics
   - table_path / sql selection or validation
   - table_list behavior
   - batch_size or split.size fallback semantics
   - partition/filter behavior
   - sharding/node selection
   - server timezone handling
   - clickhouse.config handling
   - remote connectivity or metadata checks
   - Sink behavior
   - ClickhouseFile behavior, including the delimiter work already represented 
by #12512
   
   If this Source `host`-only slice matches the intended boundary, I’ll 
implement it as one focused PR with the factory/ConfigValidator coverage above.


-- 
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]

Reply via email to