JunRuiLee commented on PR #7900: URL: https://github.com/apache/paimon/pull/7900#issuecomment-4527193411
> Good alignment with Java's CoreOptions.SCAN_MODE. A few comments: > > 1. **`startup_mode()` resolution logic**: The fallback from `DEFAULT` to `LATEST_FULL` when no scan options are present matches Java behavior. However, the `contains_key` checks use raw string literals like `"scan.timestamp-millis"` and `"scan.watermark"` instead of `CoreOptions` constants. This makes it fragile if those option keys are ever refactored. Please use the constant-based approach (e.g., `CoreOptions.SCAN_TIMESTAMP_MILLIS`) if they exist, or define them as constants. > 2. **Validation whitelist**: The `_validate_scan_mode()` method in `table_scan.py` is well-structured, but it raises `NotImplementedError` for `COMPACTED_FULL`, `FROM_CREATION_TIMESTAMP`, `FROM_FILE_CREATION_TIME`. These should probably log a warning or raise a more specific exception type that doesn't suggest a code bug (since `NotImplementedError` typically means the method should have been overridden). > 3. **Missing test coverage**: I don't see test files in this PR. The validation logic has many branches — at minimum there should be tests for: > > * `scan.mode=from-timestamp` without `scan.timestamp-millis` → error > * Conflicting options (e.g., `scan.mode=latest` + `scan.snapshot-id`) → error > * `scan.mode=default` with `scan.timestamp-millis` → resolves to `FROM_TIMESTAMP` > 4. **`FULL` deprecation**: The mapping from `FULL` to `LATEST_FULL` is correct but should there be a deprecation warning emitted? > > Please add unit tests covering the validation paths before merging. Thanks for the detailed review. I addressed the comments in the latest update. PTAL. -- 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]
